Align button to architecture goals#201
Open
lifeiscontent wants to merge 3 commits into
Open
Conversation
|
📚 Storybook preview: https://pr-201-propel-storybook.vamsi-906.workers.dev |
- Focus ring: add ring-offset-1 (focus ring style and offset is always the same per spec) - Disabled: add pointer-events-none to base (disabled state always blocks pointer events per spec) - Stretch: add stretch variant (auto/full) for full-width layout axis (adjustable per spec); expose as optional ButtonStretch prop on both ui and components tiers; add Stretch stories - Loading label: add buttonLabelClass + group to root so the label span dims (group-aria-busy:opacity-50) when the button is aria-busy, matching "label may dim" in spec; wrap children in <span> in components/button to pick up the class
The components-tier Button was composing raw elements with baked-in classes — a `<span>` carrying buttonLabelClass, a LoaderCircle with size/spin classes, and a bare NodeSlot — so styling leaked into the composition layer and the ui parts stopped at the single root <button>. Add three single-element ui parts (each with its own cva in ui/button/variants.ts): ButtonIcon (decorative leading/trailing node slot, sized to --node-size), ButtonLabel (the text, dims under aria-busy), and ButtonSpinner (the loading indicator). The components-tier Button now only composes these — no className, cx, or cva anywhere under components/button. Register the new parts as story subcomponents and add a ui Anatomy story that composes them by hand.
ButtonSpinner baked a LoaderCircle glyph; it is now a pure node-slot span that sizes and spins its single child, with the default icon moved to the components-tier Button (and to the ui Anatomy story) as explicit children.
lifeiscontent
force-pushed
the
align/button
branch
from
5906c5e to
ff32afb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #122
What the spec says
From @bhaveshraja's comment, the Figma spec breaks button properties into two categories:
Always the same (baked into the component, no prop needed): border-radius per variant, min-height/padding/font-weight/gap per size step, focus ring style and offset, disabled state (opacity + no pointer events + cursor), loading spinner, cursor when enabled, state transition timing.
Adjustable (caller-controlled): size, visual variant, label, leading/trailing icon, loading, disabled, full-width vs auto-width, icon-only mode.
What changed
Focus ring offset (
variants.ts)Added
focus-visible:ring-offset-1to the base. The spec lists "focus ring style and offset" as always the same, and the offset was missing.Disabled pointer-events (
variants.ts)Added
disabled:pointer-events-noneto the base. The spec lists "no pointer events" as always the same for the disabled state. Nativedisabledblocks interaction at the browser level but the CSS property is needed for consistency witharia-disabledscenarios.Full-width axis (
variants.ts,button.tsx, stories)Added a
stretchcva variant (auto|full) for the "full-width or auto-width" adjustable axis.stretch="full"addsw-full; omitting it (orstretch="auto") keeps the button inline-sized. Exposed as an optionalButtonStretchprop on both the ui and components tiers, and re-exported from the components index. AddedStretchstories to both tiers.Loading label dim (
variants.ts,components/button/button.tsx, stories)The spec says "label may dim" during loading. Added
groupto the button root and abuttonLabelClassexport (group-aria-busy:opacity-50) for the label span. The components tier now wrapschildrenin<span className={buttonLabelClass}>so the text fades when the button isaria-busy. The spinner itself is unaffected.Architecture notes
variants.tsvia cva andcx; component files call only the exported variants.classNameorstyleprop added to any component.defaultVariants—stretchis optional in the prop type; when omitted, cva returns""(auto sizing).vp check --no-fmtpasses clean on all 7 changed files.Needs design approval from @bhaveshraja before merge.