Skip to content

fix: close seven contract gaps reported by downstream (as-child, Empty frame, Alert variants, SheetPanel, Tabs a11y, Textarea code, sidebar anchor) - #10

Open
sheepbox8646 wants to merge 8 commits into
mainfrom
fix/design-contract-gaps
Open

Conversation

@sheepbox8646

Copy link
Copy Markdown
Member

Seven contract gaps, all reported by downstream agents who refused to hand-write around them and escalated instead — which is exactly the signal AGENTS.md § Compose, don't style asks for ("when you meet an existing component that fails this test, tell the human explicitly instead of copying the recipe one more time"). Each section below states the downstream scene, why the page layer could not legally solve it, and what the component now owns.

The acceptance test applied throughout: after this change, what does a caller still have to hand-copy? The answer has to be "nothing".


1. <Button as-child> never produced a button

Impact: largest. Long-standing defect, not a regression.

reka's Slot (what Primitive renders under asChild) merges the component's props into the first non-comment child it receives. Button always handed it a wrapper:

<span data-slot="button" data-button="" data-variant="ghost"
      class="inline-flex items-center justify-center gap-2 … h-9 px-4 py-2 … contents [gap:inherit]"
      data-button-content="">
  <a href="/x">Go</a>          <!-- class: undefined, no data-button -->
</span>

So every button class — and, worse, [data-button], the anchor every hover/press/focus rule in style.css keys off — landed on a display: contents span. It generates no box: inline-flex / h-9 / px-4 / rounded-md all evaporate, the chrome chain never connects, and the ::before fill loses its positioning ancestor (the "tints the whole surface" hazard DialogViewHeader already warns about). In Tailwind v4 contents is registered after inline-flex at equal specificity, so display: contents wins deterministically.

Silent by construction: nothing type-checks wrong, no class string is illegal, and the guard reads source text.

Attribution: the data-button-content wrapper predates 26c702e (LabelSwap / [gap:inherit]), which only added a property to an already-wrapping span. This is original behaviour, not a regression.

Fix — the asChild branch emits <slot/> and nothing else. The wrapper's three jobs, resolved rather than dropped:

Wrapper job Under as-child
[gap:inherit] forwarding to nested compositions (LabelSwap) unnecessary — the button classes incl. gap-2 are now on the caller's element, which is the parent, so the value arrives directly
data-button-content (overlay loading hides the label in place) unavailable — the caller owns the element
leading-spinner span unavailable — and actively harmful: an injected sibling is precisely what steals the merge

So loading degrades to loadingMode="manual" — the busy chrome still applies in full (data-loading holds the engaged color, clicks are swallowed), the caller renders its own indicator. Stated in the DOM (data-loading-mode="manual") and warned about in dev, never silently dropped.

Blast radius corrected. The two as-child directions are not equally affected, and the PR is careful about this:

  • <Trigger as-child><Button/></Trigger> — a reka trigger wrapping our Button (Pagination, DialogCloseButton, NumberField steppers). Never broken: the Button vnode is itself the first child. AGENTS.md § Breadcrumb & Pagination's promise held all along. Pinned by a regression test so the fix can't disturb it.
  • A component's own asChild, where its template decides what "first" is. This is the broken one: Button, and by forwarding, TextButton and BreadcrumbLink — both of which document as-child as the way to wrap a RouterLink.

New: a fourth enforcement layer, deliberately tiny. All three existing layers read source text, so they are structurally blind to "the classes are legal but on the wrong element". Adds vitest + happy-dom, tests under src/**/__tests__/ (the path tsconfig.json already excluded from vue-tsc). The 6 tests assert which element carries the contract, never what a class list says. Verified against pre-fix code: the 4 as-child tests fail, the 2 regression guards pass — so it pins the bug rather than describing the fix. AGENTS.md now carries an explicit scope limit so this can't grow into a general component test suite.

2. Empty never drew a border at all

'… rounded-lg border-dashed p-6 …'border-dashed sets border-style only. With no width the edge never rendered, so 47 downstream list pages showed an empty state as centered gray text floating in whitespace. The class string looked like it framed something, which is how it survived review.

The fix is not to add border. This repo's own page contract is explicit (skills/web/SKILL.md, reference.md § Dirty → clean): dashed is not an empty-state look — it is reserved for the "+ Add another" tile beside real items in a populated list; a fully-empty surface takes the solid frame its populated form has.

Both documented placements exist and want opposite frames, so the frame becomes an enumerated prop rather than a class pages inject:

  • variant="framed" (default) — stands in for the card/grid that appears once data exists: one solid border-border hairline at the Card radius.
  • variant="bare" — nested in a SettingsSection/Card that already draws the edge; a second stroke there is card-in-card.

Two notes for reviewers:

  • Radius moved rounded-lgrounded-xl. Not drive-by: the corner was invisible while the border was, and the § Radius role map puts container surfaces at 14px. A framed empty standing in for a Card at 10px would visibly mismatch its neighbours the moment the edge started rendering.
  • Callers relying on the borderless render: grepped — none in this repo (the only occurrence was the definition). bare reproduces the old pixels exactly for any host that did.

3. Alert had no success / warning

Downstream has an upstream-connectivity result panel. With only default and destructive, success had to render as default — i.e. unlabelled — so "check passed" and "check failed" were distinguishable only by reading the sentence.

Two rungs added, following destructive's existing shape: the frame stays neutral (bg-background + border-border) in all four and only the title/icon take the hue — one layer changing in place, § The one rule. The tinted-surface look stays CalloutBanner's job, which keeps "status message" from acquiring two competing looks. No new tokens.

The one thing worth arguing about: the text token rung is asymmetric on purpose. --destructive (L≈0.58) is dark enough to be body text on the page surface; --success (L≈0.62) and --warning (L≈0.72) are icon/fill hues and are not. Their readable-text rung is --*-foreground — the role that token was minted for and the one CalloutBanner already uses. Do not "unify" these onto the base hue for symmetry the palette deliberately doesn't have.

The new rungs also skip destructive's *:data-[slot=alert-description]:text-destructive/90: a hand-written /NN on a semantic color is § Alpha policy debt, grandfathered on that one line, not a pattern to copy. Alert comes off the § Reference status legacy list; that alpha is noted as its remaining wart.

4. No SheetPanel (the DialogPanel of the drawer)

DialogPanel exists precisely because the capped-dialog shell was a hand-copied recipe, and § Motion says that string must NEVER be hand-written. The Sheet side never got the same treatment — so downstream's ResourceFormSheet reassembles the shell at the call site, and the shape it converged on names each missing contract:

What the page wrote What was missing
sm:max-w-lg a width rung
w-[calc(100%-1rem)] narrow-screen width, re-derived per page
gap-0 on SheetContent undoing the component's own gap
border-b / border-t on SheetHeader/SheetFooter structural border painted on from outside (dirty pattern 2)
flex-row on SheetFooter the stacked mobile-nav footer is wrong for a form drawer
two class="contents" wrappers flattening the author's markup into the flow the shell should have owned

SheetPanel owns all six. Knobs: width (sm/md/lg/xl), side (left/right only — the row grid is a vertical stack, which a top/bottom sheet doesn't want), footer (adds the third row and its divider; a prop rather than slot-sniffing, for DialogPanel's reasons plus the fact that a declared-but-empty row would still draw a stray line at the panel bottom).

Ownership call worth reviewing: the header/footer hairlines and the footer's action row live on the panel as child-scoped variants (*:data-[slot=sheet-header]:border-b, …), not on SheetHeader/SheetFooter themselves. A bare Sheet — mobile nav, a filter drawer — is one flowing surface with nothing to divide, so the dividers are a property of the three-row composition. Existing bare-Sheet callers render identically. Generated CSS verified: :is(.…>*)[data-slot=sheet-header] at (0,2,0) beats the footer's own .flex-col, and the sm: rungs land inside @media (width>=40rem).

SheetBody is the scrolling middle row. Unlike DialogBody it needs no -mr-3 pr-3 gutter hack — the sheet's padding lives on the body itself, so the scrollbar already lands on the panel edge. Field rhythm stays FieldGroup's job.

5. TabsList's aria-label couldn't reach role="tablist"

TabsList's root is a wrapper div that only hosts the sliding indicator; role="tablist" is on the inner reka list. Fallthrough parked a caller's aria-label on the wrapper, and a page had no way to reach the inner element.

The downstream agent declined to write a decorative aria-label on the wrapper to satisfy an audit tool, and escalated — correctly: a name that silences the linter while the control stays anonymous to a screen reader is worse than no name.

Fixed at the mechanism, not just the symptom: inheritAttrs: false, then aria-* goes to the role-bearing inner element and everything else (id, style, listeners) stays on the wrapper — so a raw aria-label attr now also does the right thing. A label prop is the discoverable spelling and wins over a raw attr. Generalized in AGENTS.md for any future indicator-wrapping component.

6. Textarea had no variant="code"

JSON and structured-data editors could only inject class="font-mono" — the className red line. variant is a content role, not a skin: it resolves to the mono family plus a tracking reset (the base +0.01em is prose tracking for the sans stack; stacked on a monospace's already-wide advance it makes columns drift), and it's where future code-surface affordances land. Orthogonal to size.

On the "also mechanize font-* in the guard" ask: not done, and deliberately. The guard is not in this repository — it lives in the host and scans packages/ui + apps/web together with its ratchet baselines beside it. apps/web has existing font-[NNN] hits (reference.md calls it "the single most common app-page drift (60+ files)"), so the rule needs a baseline generated against the host tree, which a PR here cannot produce. Recorded in § Open migration debt with exactly that framing.

Also switches the Textarea showcase spec from a hand-copied size list to the newly exported textareaSizeKeys — the showcase doctrine forbids hand-copied option lists.

7. SidebarMenuSub's data-sidebar was a copy-paste typo

data-sidebar="menu-badge""menu-sub", copied from SidebarMenuBadge. Two unrelated components sharing one anchor means any rule or query written against it hits both. Grepped the whole repo first: nothing (CSS, query, or otherwise) depends on the wrong value. components/sidebar/ remains an unmigrated shadcn-vue import and is still not a style reference — this is a pure typo fix.

Also documented (no code change)

SidebarMenuButton's tooltip is only ever shown under <Sidebar collapsible="icon">: it is gated on state !== 'collapsed', and an offcanvas sidebar (the default) slides off the viewport when collapsed, so there's no trigger left to hover. Downstream had written three tooltips that render nowhere. Now stated on the prop and in § Reference status.


Contract & showcase

Every decision above is written back to AGENTS.md (§ Extending this contract: "a decision that is not written here will be re-invented"), including a new top-level § as-child section — a fifth invisible mechanic alongside the four cascade planes. The § Enforcement table now names the guard as living in the host repo, which is why node scripts/check-ui-contract.mjs doesn't exist in a standalone clone of this module.

Showcase gains pages for Alert, Empty and Sheet, plus new examples/controls on Button (as-child), Textarea (variant) and Tabs (label). All control options come from the components' exported *Keys arrays.

Verification

$ pnpm type-check
> vue-tsc --build
(clean; also re-run with --force)

$ pnpm test
 Test Files  1 passed (1)
      Tests  6 passed (6)

$ node scripts/check-ui-contract.mjs        # host script, this tree symlinked in as packages/ui
⚠ UI contract — 2 warning(s):
  packages/ui/src/components/sidebar/Sidebar.vue:66  raw shadow utility … → group-data-[variant=floating]:shadow-sm
  packages/ui/src/components/sidebar/SidebarInset.vue:15  raw shadow utility … → md:peer-data-[variant=inset]:shadow-sm
ℹ alpha baseline: 16 grandfathered hand-written alpha value(s) remaining
✓ UI contract OK (2 warning(s))
guard exit=0

The two warnings are byte-identical on origin/main (verified by running the same guard against a pristine git archive of the branch point) — pre-existing debt in the unmigrated sidebar directory, untouched here. pnpm build-only also succeeds, and the generated CSS was inspected to confirm the new *:data-[slot=…] variants, --font-mono, text-success-foreground/text-warning-foreground and the panel's arbitrary width/grid values all emit real rules.

🤖 Generated with Claude Code

sheepbox8646 and others added 8 commits July 29, 2026 15:32
`<Button as-child><a/></Button>` never produced a button. reka's Slot merges
the component's props into the FIRST non-comment child it receives, and this
component always handed it a `<span data-button-content class="contents">`
wrapper — so `inline-flex` / `h-9` / `px-4` / `rounded-md` and, worse, the
`[data-button]` anchor that every hover/press/focus rule in style.css keys off,
all landed on a span with `display: contents`. It generates no box, so the
caller's `<a>` / `<RouterLink>` rendered as bare text and the `::before` fill
lost its positioning ancestor.

Silent by construction: nothing type-checks wrong, no class string is illegal,
and the guard reads source text. Long-standing, NOT a regression — the wrapper
predates the LabelSwap `[gap:inherit]` change, which only added a property to
an already-wrapping span.

Fix: the asChild branch emits `<slot/>` and nothing else. The wrapper's two
jobs don't survive the move and don't need to — the button classes (gap
included) are now on the caller's element, so a nested LabelSwap's
`[gap:inherit]` reads the real gap from its actual parent; and `loading`
degrades to `loadingMode="manual"` (busy chrome, caller-rendered indicator),
reported in the DOM and warned about in dev rather than silently dropped.

Also fixes `<TextButton as-child>` and `<BreadcrumbLink as-child>`, which
forward straight into Button and are both documented as the way to wrap a
RouterLink. The other as-child direction — `<PaginationItem as-child><Button/>`
— was never broken and is pinned by a regression test.

Adds a deliberately narrow DOM-test layer (vitest + happy-dom, tests under
`src/**/__tests__/`, already excluded from `vue-tsc`) because this defect class
is invisible to all three existing enforcement layers: they read source text,
and the bug is about which ELEMENT carries the contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Empty shipped `rounded-lg border-dashed`. `border-dashed` sets border-STYLE
only — with no width the edge never rendered, so every standalone empty state
was centered gray text floating in whitespace. The class string looked like it
framed something, which is why it survived review.

The fix is not `border`: the page contract (skills/web) is explicit that dashed
is NOT an empty-state look — it's reserved for the "+ Add another" tile beside
real items in a populated list. A fully-empty surface takes the SOLID frame its
populated form has.

Both documented placements exist, and they want opposite frames, so the frame
becomes an enumerated prop instead of a class pages inject:

- variant="framed" (default) — stands in for the card/grid that appears once
  data exists: one solid `border-border` hairline at the Card radius
  (rounded-xl, per the radius role map — the dead `rounded-lg` was the control
  rung and would have visibly mismatched adjacent cards once the edge rendered).
- variant="bare" — nested inside a SettingsSection / Card that already draws
  the hairline; a second stroke there is card-in-card.

No caller in this repo depended on the borderless rendering; `bare` reproduces
it exactly for hosts that did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With only `default` and `destructive`, a result panel could not distinguish
"check passed" from "check failed" — success had to render as `default`, i.e.
unlabelled. A reader had to finish the sentence to learn the outcome.

Both new rungs follow destructive's shape: the frame stays neutral
(`bg-background` + `border-border`) in all four and only the title/icon take
the hue — one layer changing in place. The tinted-surface look stays
CalloutBanner's job, so "status message" keeps one look per role.

Token rung is asymmetric on purpose: `--destructive` (L≈0.58) is dark enough
for body text on the page surface, `--success` (L≈0.62) / `--warning` (L≈0.72)
are icon/fill hues and are not — their readable-text rung is `--*-foreground`,
the role that token was minted for and the one CalloutBanner already uses. No
new tokens.

The new rungs deliberately skip destructive's `text-destructive/90` description
tint: that hand-written alpha is grandfathered debt, not a pattern to copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DialogPanel exists because the capped-dialog shell was a hand-copied class
string. The Sheet side never got the same treatment, so every form drawer
reassembled the shell at the call site — and the shape it converged on names
each missing contract one by one: a `sm:max-w-*` the page had to invent,
`gap-0` undoing the component's own gap, `border-b`/`border-t` injected onto
SheetHeader/SheetFooter (a structural border painted on from outside),
`flex-row` turning the stacked mobile-nav footer into an action row,
`w-[calc(100%-1rem)]` for narrow screens re-derived per page, and
`class="contents"` wrappers whose only job was flattening the author's markup
back into the flow the shell should have owned.

SheetPanel owns all of it. Knobs: `width` (sm/md/lg/xl — add a rung here, not
per page), `side` (left/right only; the row grid is a vertical stack, which a
top/bottom sheet doesn't want), `footer` (adds the third row AND its divider —
a prop rather than slot-sniffing, for DialogPanel's reasons plus the fact that
a declared-but-empty row would still draw a stray line).

The header/footer hairlines and the footer's action row live on the panel as
child-scoped variants, not on SheetHeader/SheetFooter: a bare Sheet (mobile
nav, a filter drawer) is one flowing surface with nothing to divide, so the
dividers belong to the three-row COMPOSITION. Existing bare-Sheet callers are
untouched.

SheetBody is the scrolling middle row. Unlike DialogBody it needs no
`-mr-3 pr-3` gutter hack — the sheet's padding lives on the body itself, so
the scrollbar already lands on the panel edge. Field rhythm stays FieldGroup's
job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TabsList's root is a wrapper div that exists only to host the sliding
indicator; `role="tablist"` is on the inner reka list. Default attribute
fallthrough parked a caller's `aria-label` on the wrapper, where no screen
reader reads it — and a page had no way to reach the inner element. That
failure is worse than no label: the audit tool goes green while the tab row
stays anonymous.

Splits $attrs deliberately (inheritAttrs: false): aria-* to the role-bearing
inner element, everything else (id, style, listeners) to the wrapper. Adds a
`label` prop as the discoverable spelling, which wins over a raw attr.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JSON / config / prompt-template editors had no way to say what they hold, so
the only route was injecting `class="font-mono"` — the className red line. The
guard happens not to detect `font-*` yet, so the rule was written but not
mechanized.

`variant` is a content ROLE, not a skin. It resolves to the mono family plus a
tracking reset (the base +0.01em is prose tracking for the sans stack; stacked
on a monospace's already-wide advance it makes columns drift), and it is the
hook future code-surface affordances hang off. Orthogonal to `size`.

Also switches the showcase spec's size options to the newly exported
`textareaSizeKeys` — the spec had a hand-copied list, which the showcase
doctrine forbids.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It carried `data-sidebar="menu-badge"`, copy-pasted from SidebarMenuBadge.
Two unrelated components sharing one anchor means any rule or query written
against it hits both. `menu-sub` matches the component and its data-slot.

Verified no code — CSS, query or otherwise — depends on the wrong value.

Also documents that SidebarMenuButton's `tooltip` is only ever shown under
`<Sidebar collapsible="icon">`: it is gated on state === 'collapsed', and an
offcanvas sidebar (the default) slides off the viewport when collapsed, so
there is no trigger left to hover. Without the note, callers keep writing
copy that renders nowhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A decision that is not written here gets re-invented (§ Extending this
contract). Adds:

- § as-child — a new top-level mechanic, sibling to the four cascade planes:
  reka's Slot merges into the FIRST non-comment child, so a component that
  forwards asChild must render the slot and nothing else. Includes the two
  directions (only one is fragile), and the rule that a capability needing an
  injected node must degrade explicitly.
- § Empty — the frame is a prop (framed / bare), dashed is never an empty look,
  and the radius rung it belongs to.
- § Alert vs CalloutBanner — semantic through TEXT vs through SURFACE, plus why
  the text token rung is not symmetric across hues.
- § Textarea variant="code" — machine text declares itself through a prop, and
  how it differs from Badge's `font` axis.
- § aria-* forwarding — a wrapper must not swallow the accessible name of the
  element that carries the role.
- SidebarMenuButton's tooltip is only effective under collapsible="icon".
- Enforcement table: names the guard as living in the HOST repo (it scans
  packages/ui + apps/web together and keeps its baselines there, so it does not
  run from a standalone clone of this module), and adds the narrow fourth
  layer with an explicit scope limit so it can't grow into a component test
  suite.
- Open debt: the guard does not detect `font-*` injection — rule written,
  not mechanized; needs a host-side rule plus a ratchet baseline.

Registers the Alert / Empty / Sheet showcase pages.

Also points skills/web at the new Empty prop, so page authors stop reaching
for a border class the component now owns.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sheepbox8646
sheepbox8646 requested a review from a team as a code owner July 29, 2026 07:35
@qqqqqf-q

Copy link
Copy Markdown
Member

Review notes from a local checkout of c9cfaf28:

  1. Button as-child does not currently preserve the promised loading/disabled interaction contract. isDisabled is only set when !asChild && as === "button", so an <a> / RouterLink rendered through as-child receives data-loading and aria-busy but remains navigable/clickable. That conflicts with the component docs and comments, which say loading/manual mode blocks clicks. The new test only checks DOM attributes and spinner absence, so it does not catch the active navigation. Please add explicit non-native/as-child inert semantics (aria-disabled plus click/navigation prevention) and a test that actually triggers the click.

  2. The new DOM contract tests are not wired into CI. package.json adds pnpm test, but .github/workflows/ci.yml still runs only install and type-check. As written, the proposed fourth enforcement layer is local-only and GitHub can remain green after an as-child regression. Please add pnpm test to the check job.

Rollout note, not necessarily a blocker to merging this UI PR by itself: Empty now defaults to framed, while the Memoh host currently has multiple Empty instances nested inside SettingsSection without variant="bare". Because the host pins this repository as a submodule, those callers should be migrated before the pointer bump, or the default should remain backward-compatible; otherwise the host will gain nested/double frames.

The overall contract direction looks good. I would approve after items 1 and 2 are addressed, with the Empty host migration coordinated before the Memoh submodule update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants