feat(core): let CheckboxInput and Switch name their own label theme target - #5183
Open
freddymeta wants to merge 4 commits into
Open
feat(core): let CheckboxInput and Switch name their own label theme target#5183freddymeta wants to merge 4 commits into
freddymeta wants to merge 4 commits into
Conversation
…ol labels
FieldLabel always emitted themeProps('field-label') with no variant, so a theme could not distinguish a label stacked above a field from one sitting beside its control (CheckboxInput, Switch). Add a 'layout' prop ('stacked' | 'beside', default 'stacked') reflected as data-layout, and pass 'beside' from CheckboxInput and Switch. Themes can now drop the stacked bottom margin for beside-control labels so the text stays vertically centered.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsCheckboxInput (@astryxdesign/core) · View in Storybook
Field (@astryxdesign/core) · View in Storybook
Switch (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
requested changes
Aug 19, 2026
cixzhang
left a comment
cixzhang
left a comment
Contributor
There was a problem hiding this comment.
Thanks — a theme genuinely can't tell a checkbox label from a form label today, so this is a real gap.
I'd rather not solve it with a prop, though. layout changes nothing but the data- attribute, so a consumer can pass layout="beside" on a stacked label and the theme quietly lies. It's also not theirs to know: CheckboxInput and Switch already know they're a control row.
Could the control pass a target down instead?
<FieldLabel {...themeProps('checkbox-label')} … />Then a theme names the thing it wants rather than knowing an encoding, nobody can set it untruthfully, and it matches how the other targets landed recently.
It also sidesteps a case this misses: Field direction="horizontal-labels" puts a label beside its control and still reports stacked, so the stacked bottom margin lands on it — I applied it in Chrome and those labels drift off their inputs.
[Reviewed by Robohands, edited by Cindy]
…arget
Replaces the `layout` prop with the shape review asked for. `layout` was an
encoding the label could not verify: a consumer could pass `layout="beside"`
on a stacked label and the theme would quietly believe it, and the label has
no way to know how its caller arranged it. Three placements exist, not two —
`Field direction="horizontal-labels"` also sits a label beside its control and
would have reported `stacked`, so a theme's stacked rule would land on it.
The control passes its own target down instead: CheckboxInput spreads
`themeProps('checkbox-label')` and Switch `themeProps('switch-label')` into
FieldLabel, which composes them onto the `astryx-field-label` it already
renders. A theme now names the thing it wants rather than an arrangement,
nobody can set it untruthfully, and the horizontal-labels case needs nothing
because no label claims a placement any more.
Both targets are documented on their own component (EN + zh), which is also
what makes them checked: themingTargets.test.ts fails if a rendered
`astryx-*` class is missing from that component's doc — verified by removing
each entry in turn. Reverts the `visualProps: ['layout']` this had added to
`astryx-field-label` in Field.doc.mjs.
…targets
`ComponentThemingTarget` is `{className, visualProps?, states?,
deprecatedFor?}` — no `description`. `pnpm -F @astryxdesign/core typecheck:docs`
rejects the extra key, and that is a SEPARATE gate from the `typecheck` I ran:
both live in the build-storybook job, which is why the docs typecheck is the
one that caught this. The two targets carry a class name only, like
`astryx-card`; what they are for is in the changeset.
Also merges current main (clean) so the branch is checked against it.
freddymeta
changed the title
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
3 participants
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.
What
CheckboxInputandSwitchnow name their own label as a theme target —astryx-checkbox-labelandastryx-switch-label— alongside theastryx-field-labelevery label already carries.A theme could previously only reach one
astryx-field-labeltarget, so stylinga checkbox's label (which shares a row with its control) meant styling every
form field's label above its input too.
Why not the
layoutprop this PR started withReview was right to hold it, and for a reason that went further than the API
shape:
layoutwas an encoding the label could not verify. A consumer could passlayout="beside"on a stacked label and the theme would quietly believe it.Field direction="horizontal-labels"also sits a label beside its control, and would have reported
stacked, so atheme's stacked rule (say, a bottom margin) would land on it and drift the
label off its input.
The control knows what kind of label it owns, so it passes the target down:
FieldLabelcomposes it onto theastryx-field-labelit already renders. Thename says what the thing is rather than how it is arranged, nothing can set it
untruthfully, and the horizontal-labels case needs nothing at all — no label
claims a placement any more.
Test plan
and
FieldLabelasserts it emits nodata-layoutof its own. Removing eachthemeProps(...)spread fails exactly the matching test — verified in bothdirections.
makes them checked:
themingTargets.test.tsfails when a renderedastryx-*class is missing from that component's doc. Verified by removingeach doc entry in turn.
pnpm test533 files / 10899 tests,lint:strict0 errors,pnpm build,and every typecheck the
build-storybookjob runs — includingtypecheck:docs, which is a separate gate fromtypecheckand the one thatcaught an unsupported
descriptionkey on a theming target.visualProps: ['layout']this had added toastryx-field-labelin
Field.doc.mjs. Currentmainis merged in.