overhaul: 03 primitives - #17
Conversation
|
CS-5
left a comment
There was a problem hiding this comment.
Correctness review of the primitives layer. I built the branch and inspected the generated HTML/CSS to check each of these rather than reasoning from the source alone; pnpm check and pnpm build are both clean on the branch, so everything below is runtime/behavioral.
Two look blocking:
- Carousel's scoped
<style>never matches its slides. Astro requires the component's scope attribute on the child, and slotted content doesn't have it — so no snap alignment, no slide sizing, anditemBasisis dead. The component's headline feature doesn't work. FieldError'sempty:hiddencan never match, because the always-rendered<Icon>means the<p>is never:empty. An empty placeholder renders a bare red warning triangle.
The rest: disabled silently does nothing on Button's anchor path; Accordion's name prop never reaches its items (so the wrapper's documented exclusive-open is a no-op, and the shadcn-astro skill's worked example describes behavior that isn't there); Button can't type target/rel on the href path, which every external link button will need; filled icons get a 2px stroke painted over the fill; the Carousel arrows opt out of the global reduced-motion switch; and /styleguide now numbers its sections 1–6, 8, 7.
Nothing here argues with the architecture — the ladder, native <dialog>, and the cn() font-size fix all look right, and the text-body/text-copy split does resolve the way the docs claim (verified in the compiled CSS).
Generated by Claude Code
3e795d7 to
883ccf3
Compare
883ccf3 to
3214019
Compare
|
3214019 to
5c9981c
Compare
5c9981c to
9f5eb18
Compare
The zero-JS primitive layer: Button, Card and its sub-parts, Badge, Input,
Textarea, Label, FieldError, Separator, Accordion, Dialog, Carousel, Icon,
Skeleton — every one on /styleguide in all variants and all three themes.
Interactivity stays as high up the ladder as it can: Accordion is native
<details> with the name attribute for exclusive open, Dialog is native
<dialog> so the browser owns the focus trap and Esc, Carousel is a
scroll-snap track. Together the whole styleguide ships 835 bytes of inlined
script, and a page built from only the static primitives ships none.
Two silent bugs surfaced here, both of which would have spread across every
page:
cn() was dropping font sizes. Tailwind builds text-* utilities from both
--text-* and --color-*, and the merge step only knows Tailwind's stock scale,
so it treated every text-* class as one conflict group and kept the last.
cn("text-primary-foreground", "text-body") collapsed to text-body, which
rendered every primary button's label in body gray on Safety Yellow — 1.3:1,
measured in the browser. @/lib/cn now registers the DESIGN.md §3 type scale as
the font-size group; those buttons measure 11.7:1, and size-plus-color pairs
keep both classes.
Relatedly, text-body is a color and not a size, so Button, Input, and Textarea
were asking for a size and getting none. They use text-copy now.
Icons follow ADR 0002: @tabler/icons inlined at build, no astro-icon or
Iconify. The package's exports map rewrites every subpath including
package.json, so the icons directory is located through a known icon instead.
CVA recipes live in sibling *.variants.ts files, since Astro forbids exporting
values from a component. That also lets one component reuse another's recipe.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YRfxMh7FLjQtDbb1BEsCbR
Four primitives had behavior that was declared but never ran. Carousel's scoped `[data-carousel-track] > *` was rewritten by Astro to require this component's own scope attribute, which slotted slides never carry — so `scroll-snap-align`, `flex: 0 0 100%` and the `flex-basis` media rule matched nothing. Slides collapsed to content width, nothing snapped, and `itemBasis` was dead. `:global(*)` on the child escapes the scope while keeping `define:vars`. FieldError's `empty:hidden` could never match: `:empty` requires no child nodes and the `<Icon>` was unconditional, so a placeholder rendered a bare red alert triangle. The icon is now gated on slot content. Button spread `disabled` onto the `<a>` path, where the attribute is invalid and `:disabled` never matches — a disabled link rendered at full opacity, fully clickable, and typechecked cleanly. It maps to `aria-disabled` (which the variants already style) and the `href` is dropped. `Props` also extended only `HTMLAttributes<"button">`, so every external link button the site needs failed `astro check`; it now picks up `download`/`hreflang`/`rel`/`target`. Accordion's `name` became an unused `data-accordion-name` with no consumer, so the wrapper's documented exclusive-open was a no-op — the styleguide worked only because it repeated `name` on each item. Dropped the prop; the JSDoc and the `shadcn-astro` worked example now say where `name` belongs. Icon emitted `stroke`/`stroke-width` unconditionally, but Tabler's filled sources carry no stroke, so filled glyphs inflated ~1px on every edge and thickened narrow details. Carousel's explicit `behavior: "smooth"` bypassed the reduced-motion `scroll-behavior: auto !important` in global.css; the track's `scroll-smooth` class supplies it instead. The styleguide's section numerals ran 1–6, 8, 7. Quality: `Field.variants.ts` holds the recipe `Input` and `Textarea` were copy-pasting — five of six lines byte-identical, which is the reuse the sibling-variants convention exists for. `Button` gained a `pocket` variant so the Carousel arrows and the Dialog close compose it rather than hand-building icon buttons; the Dialog's was `p-1`, below the 44px minimum the shared recipe enforces. `tools/checks/cn-font-size-group.mjs` makes `cn`'s font-size group drifting from the `--text-*` tokens a `pnpm check` failure instead of a comment — that invariant fails invisibly, and it already cost this phase a 1.3:1 contrast bug. The brief's carousel keyboard criterion had been rewritten in place to describe what shipped. Restored, with the arrow decision recorded under "Deviations from this brief" alongside the rest. Newly-live jsx-a11y rules (see PR #15) caught two real violations: `href="#"` on the styleguide's demo links, and `tabindex="0"` on the carousel track. The track is a keyboard-reachable scroll container, so it is `role="region"` with a name, and that one role is added to `no-noninteractive-tabindex`'s allowlist — dropping the tabindex would make the slides keyboard-unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BX5PrKuYNRLVxiEj3eejhs
9f5eb18 to
4aae3a1
Compare
Layer 3 of the overhaul stack, on
overhaul/02-design-system.plan/03-primitives.md.The zero-JS primitive layer, plus the agent skill for adding more.
What's here
Button(5 variants × 4 sizes, renders<a>givenhref),CardwithCardHeader/Title/Description/Content/Footer,Badge(7 tones incl. the four sponsor tiers),Input,Textarea,Label,FieldError,Separator,Accordion+AccordionItem,Dialog,Carousel,Icon,Skeleton. Every one is on/styleguidein all variants and states, and the theme section at the bottom now renders the full set underdata-theme="frc"and"fll".Interactivity stays as high up the ladder as it can go: Accordion is native
<details>with thenameattribute for exclusive open, Dialog is native<dialog>so the browser owns the focus trap and Esc, Carousel is a CSS scroll-snap track. The whole styleguide — Dialog and Carousel included — ships 835 bytes of inlined script, and a page built from only the static primitives ships zero<script>tags and zero.jsfiles.Also here:
src/components/ui/primitives/README.md(the conventions) and.claude/skills/shadcn-astro/SKILL.md(how to port a shadcn component, with the Radix→native mapping table and the Accordion worked through end to end, including what gets dropped and why).Both would have spread through every page built after this one.
1.
cn()was dropping font sizes.Tailwind builds
text-*utilities from two token namespaces —--text-*for sizes,--color-*for colors — and the merge step only recognizes Tailwind's stock scale (text-sm,text-lg), nottext-h4ortext-small. So it treated everytext-*class as a single conflict group and kept only the last one:The first line is the damaging one. Every primary button's label rendered in body gray on Safety Yellow — I measured it in the browser at 1.3:1.
@/lib/cnis now a configured merge (cnfast takes a tailwind-merge config) that registers the §3 type scale as the font-size group. Same buttons now measure 11.7:1, and size-plus-color pairs keep both classes while two colors or two sizes still resolve to the last.The tradeoff worth flagging: adding a size token to
global.cssnow means adding one line tocn.ts, or it silently loses to any color next to it. That's called out incn.ts, the primitives README, and the skill.2.
text-bodyis a color, not a size. Phase 02 established this (bodynames both, Tailwind resolves colors first), but Button, Input, and Textarea were all usingtext-bodyintending the size and silently getting none. They usetext-copynow.Deviations from the phase brief
@tabler/icons, inlined at build per ADR 0002 — notastro-icon+@iconify-json/tabler. Worth knowing: that package'sexportsmap is"./*": "./icons/*", which rewrites every subpath includingpackage.json, sosrc/lib/icon.tslocates the icons directory through a known icon file rather than the manifest. An unknown icon name throws at build with a pointer to tabler.io/icons.*.variants.tsfiles. Astro forbids exporting values from a component (astro/no-exports-from-components), sobuttonVariantscan't live inButton.astro. This turns out better than shadcn's arrangement: another component can import the recipe rather than copying classes.Skeletonincluded, noSpinner— the calendar's loading state needs the former in Phase 07; nothing needs the latter.Verified
pnpm check && pnpm buildgreen.dialog.openistrueafter the trigger,falseafter Escape.cn()fix (11.71:1; 14px / 16.85px / 18.70px for sm/md/lg, the fluid clamp working).Generated by Claude Code