feat(theme): add themeable indicators - #4712
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
cixzhang
changed the title
0f08ef8 to
3fa9628
Compare
|
Heads up on an overlap with #4724 ( They're solving adjacent problems — #4724 extracts the radio input + visual so a single radio can render outside a |
61535c1 to
4fb13b6
Compare
b3b2c26 to
4e7a7ee
Compare
4e7a7ee to
6fa7871
Compare
github-actions
Bot
added
the
needs:design-review
cixzhang
changed the title
|
Rescoped to the minimal API, and retargeted to Dropped: Size: 82 files / +2850 → 26 files / +1574. Four changesets → one, 57 words. Also removed a wrapper I'd added. The first pass had On the test failures: the full UI suite shows 5, in Table perf and TransferList draft timing. I ran pristine Not yet done: visual before/after screenshots. Doing those next unless you want to look at the API first. |
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR New ComponentsIndicator (@astryxdesign/core)
Modified ComponentsCheckboxInput (@astryxdesign/core) · View in Storybook
DropdownMenu (@astryxdesign/core) · View in Storybook
RadioList (@astryxdesign/core) · View in Storybook
Selector (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. RadioList - 1 issue(s)
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
| // The focus ring lives on the wrapper — which owns the (visually hidden) | ||
| // native input — rather than on the checkbox visual, so replacing the | ||
| // checkbox indicator through `defineTheme({indicators})` cannot leave the | ||
| // control without a visible focus indicator (WCAG 2.4.7). Mirrors | ||
| // RadioListItem's radioWrapperFocus. | ||
| checkboxWrapperFocus: { | ||
| outline: { | ||
| default: 'none', | ||
| ':has(:focus-visible)': `2px solid ${colorVars['--color-accent']}`, | ||
| }, | ||
| outlineOffset: { | ||
| default: '0', | ||
| ':has(:focus-visible)': '2px', | ||
| }, | ||
| borderRadius: radiusVars['--radius-inner'], | ||
| }, |
There was a problem hiding this comment.
This styling belongs on the indicator now.
There was a problem hiding this comment.
Moved — and you were pointing at a real bug, not just a placement preference.
The ring can't use a plain :focus-visible on the indicator: the native input is a visually hidden sibling, not a descendant. So it now arrives the same way hover already does, through the owner's indicatorScope marker:
outline: {
default: 'none',
[stylex.when.ancestor(':has(:focus-visible)', indicatorScope)]: `2px solid ${colorVars['--color-accent']}`,
},The bug it fixes: with the ring on the wrapper, the host had to hardcode the indicator's shape. RadioListItem literally carried borderRadius: '50%' for a circle it doesn't own, and CheckboxInput carried --radius-inner. Replace either indicator with a different shape and the ring traced the wrong outline. Now each indicator draws its own — measured in Chrome after the move: checkbox 2px solid, offset 2px, radius 6px; radio 2px solid, offset 2px, radius 50%.
The tradeoff, stated plainly: a replacement that doesn't draw a ring now ships a control with no visible focus (WCAG 2.4.7) — previously the wrapper covered for it. I flipped the rule in Indicator.doc.mjs from "don't draw your own focus ring" to "draw it, keyed off the scope marker," with the sibling-input reason. Worth noting the vibe test in #4831 measured 4/4 replacements honoring explicitly-stated contract rules, which is the evidence that documenting it works.
Tests updated: the ring is asserted on the indicator, plus a regression guard that no host reintroduces one (two rings, or one of the wrong shape).
There was a problem hiding this comment.
Right — that was a bad answer on my part. Callsites shouldn't, and now they don't.
I looked at the two built-in rings and they were byte-identical, and neither mentions a shape. That's the tell: outline follows the element's own border-radius automatically, so the rule is completely generic. There was never anything per-indicator in it, which means "write your own" was me exporting a chore.
So it's one exported style now, next to indicatorScope:
import {indicatorFocusRing} from '@astryxdesign/core/Indicator';
<span {...stylex.props(indicatorFocusRing, myStyles.box)} aria-hidden="true" />The built-ins spread exactly that. A replacement spreads the same one line and gets a ring that matches its own shape for free — the doc rule now points at the export instead of describing rules to write.
It still has to live on the indicator rather than the owner, for the reason the original comment was getting at: the owner would have to hardcode the shape of whatever it hosts. And it keys off the scope marker because the native input is a hidden sibling. But neither of those is now the replacement author's problem.
One case that fell out: CheckIndicator deliberately doesn't take the ring. A check marks a chosen row in a listbox or menu, and the row takes focus, not the mark. I noticed because adding it broke the existing test asserting the default check renders byte-identically to a plain <Icon> — the test was right.
Re-measured in Chrome after the refactor, unchanged: checkbox 2px solid / offset 2px / radius 6px, radio 2px solid / offset 2px / radius 50%.
| themeProps('dropdown-menu-checkbox', { | ||
| size: controlSize, | ||
| checked: value ? 'checked' : null, | ||
| disabled: isDisabled ? 'disabled' : null, | ||
| }), |
There was a problem hiding this comment.
Hmm should this be on the CheckboxControl instead? Themes should collapse onto the visible element. Also we apply size on this extra span which seems redundant to the CheckboxControl
There was a problem hiding this comment.
Both right, and the wrapper is gone.
The size was pure duplication — markerSizeStyles was {sm: 20×20, md: 24×24}, byte-identical to the indicator's own boxSizeStyles. Item renders {marker} with no wrapper of its own, so the span had nothing left to do; what remains (order, coarse-pointer placement, pointerEvents: none) is genuinely the menu's business and rides xstyle on the indicator.
One correction while doing this: I had put a dropdown-menu-checkbox target on it, and that would have been a new theme target — main doesn't declare one (only dropdown-menu-radio and -radio-dot are in DropdownMenu.doc.mjs). I checked what main actually renders on that element and it's astryx-checkbox, which is still there. So the menu checkbox adds no target at all. The radio keeps dropdown-menu-radio, which did pre-exist, now on the painted circle instead of a box around it.
Visual diff vs main with the menu open: menu-checkbox-items is 0px changed; the radio stories differ by the disclosed 22→24px control-size alignment.
| @@ -205,24 +178,13 @@ export function DropdownMenuRadioItem({ | |||
| checked: isChecked ? 'checked' : null, | |||
| disabled: isDisabled ? 'disabled' : null, | |||
| }), | |||
There was a problem hiding this comment.
Same comment here as above for the dropdown menu checkbox item.
There was a problem hiding this comment.
Same change here — wrapper removed, dropdown-menu-radio moved onto the painted circle (it pre-existed on main, so it stays), and the duplicated size styles are gone.
This is the one place with a real pixel change: the menu radio was a bespoke 18/22px and now uses the standard 20/24px, so it matches a RadioList radio. Measured 1.8% of pixels in lab-radio-group and 0.4% in lab-selectable-sizes; every other story I diffed is 0px.
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Can we get a storybook for swapping the selector's single selection indicator to use the radio?
|
All four addressed in Storybook for the radio swap — added two stories side by side, both opened by default so the listbox is visible:
That contrast is the whole argument for rendering the mark unconditionally — the empty circles only exist because the indicator is told its state instead of being conditionally rendered. Visual verification. Ran both Storybooks (this branch and pristine
The two non-zero ones are the disclosed 22→24px menu radio alignment. Everything else is byte-identical. Two things I got wrong before the numbers were trustworthy, both caught by checking rather than by the numbers looking plausible: the first run compared two "Couldn't find story" pages (I'd guessed story IDs), and the second compared two closed menus because my click was wrapped in a silent Test suite: 8 failures on the full run, all in perf/timing files (Table perf, Markdown perf, TransferList ×5, submenu roving). Pristine main on the same machine gives 5, from the same set, and the count swings run to run under load. Every component this PR touches — Indicator, CheckboxInput, RadioList, DropdownMenu, Selector — is 256/256 passing in isolation. |
Indicators are the componentized form of stateful control visuals — the
checkbox box, the radio circle, the mark on a chosen option. The owning
component keeps the input, role, accessible name, focus, and keyboard
behavior; the indicator turns state into a picture.
Replacement is by NAME through defineTheme({indicators}), so one entry
reaches every component that draws that indicator: mapping `check` to
RadioIndicator gives radio visuals to every single-selection mark.
- Families (singleSelection, multiSelection) fix the state space, each
using the ARIA vocabulary for the state it expresses (#4831). The
registry is family-checked, so a replacement cannot be written against
the wrong states.
- CheckboxInput, RadioList, and the DropdownMenu selectable items render
the shared indicators; ~450 lines of duplicated control styling go.
- Selector renders its mark unconditionally with state, so a replaced
indicator can draw its unselected form (a radio's empty circle) — the
default check still draws nothing when unchecked.
- CheckIndicator renders <Icon> directly rather than wrapping it, so a
host's theme target lands on the glyph, and it adds no new theme target
of its own.
- The focus ring moves to the wrapper owning the visually-hidden input,
so replacing an indicator cannot leave a control without a visible
focus indicator (WCAG 2.4.7).
Deliberately excluded: component icon/indicator slot maps. Replacement
by name covers the known cases; slots stay additive for when it doesn't.
…ppers Review feedback: styling that describes the visible control belongs on the control, not on a wrapper around it. - The focus ring moves from the host wrappers to the indicators, keyed off the owner's indicatorScope marker (the input is a visually hidden SIBLING, so a plain :focus-visible can never match). This also removes the hosts' hardcoded guesses about indicator shape — RadioListItem had `borderRadius: 50%` for a circle it does not own. Measured after the move: checkbox ring 6px, radio ring 50%, each from its own indicator. Drawing the ring is now part of the documented replacement contract. - The DropdownMenu marker wrappers are gone. Their size styles duplicated the indicator's own control size exactly (20/24), and the theme target now lands on the visible box. - No new theme target for the menu checkbox: main reached that element through `astryx-checkbox`, which is still on it. `dropdown-menu-radio` pre-existed and moves onto the circle. - Storybook: RadioSelectionIndicator + DefaultSelectionIndicator show the check -> radio swap side by side. Visual diff vs origin/main, both Storybooks running the same stories: checkbox variations, indeterminate, and radiolist are 0px changed; menu radio group differs 1.8% and selectable sizes 0.4%, which is the disclosed 22->24px control-size alignment. Verified the harness detects change with a positive control, after an earlier run compared two 'story not found' pages and two closed menus.
…echeck Replacements should not hand-write focus rules. The two built-in rings were byte-identical and mention no shape — outline follows the element's own border-radius — so there was never anything per-indicator in them. - `indicatorFocusRing` is exported alongside `indicatorScope`. Built-ins spread it; a replacement spreads the same one line and is done. - CheckIndicator deliberately does NOT take it: a check marks a chosen row in a listbox or menu, and the row takes focus, not the mark. Caught by the test asserting the default check renders byte-identically to a plain <Icon>. - Docs point at the exported style instead of describing rules to write. Also two CI failures from the previous commit, both from checks I had not run locally: - `scripts/sync-exports.js` — the new Indicator/ subpath needed a package.json export entry. - `pnpm -F @astryxdesign/storybook typecheck` — my new stories typed value as `string | null`, which Selector only accepts with hasClear. Verified in Chrome after the refactor: checkbox ring 2px/offset 2px/ radius 6px, radio 2px/offset 2px/radius 50% — unchanged.
Removing the marker wrapper took the themeProps call with it, but left the target declared in DropdownMenu.doc.mjs — a documented target with no element behind it. build-theme.registry.test.mjs catches exactly this; I had not run the node project locally. The declared list is now main's minus dropdown-menu-radio-dot (the documented removal), with nothing added.
radiusVars in CheckboxInput (the wrapper ring used it), and Theme / defineTheme / IndicatorProps in the test (the old replacement-survives test used them). Strict lint reports these as warnings, so CI stayed green while they accumulated.
Three defects, all surfaced by asking why the doc said group: 'Checkbox'. - group was 'Checkbox', which picks one of three indicators arbitrarily and makes the docsite nest the family under CheckboxInput while MCP offers CheckboxList as related. Indicator is its own family with three components, like TreeList or Selector, so it groups as 'Indicator'. - CheckIndicator was undocumented — the component whose replacement is the headline use case of the whole feature. - CheckboxIndicator and RadioIndicator took bare IndicatorProps, whose default is the union of every family, so RadioIndicator advertised an 'indeterminate' state its family cannot express. It typechecked (contravariance: accepting more states is assignable where fewer are passed), which is why nothing caught it — the family parameter was decorative on the components that define the families. Now IndicatorProps<'multiSelection'> and <'singleSelection'>, and the doc matches.
Two findings from an independent audit against the component rubric,
plus the target rename.
BLOCK — a themed indicator could ship with no visible focus (WCAG
2.4.7). Measured: the ThemedIndicator story rendered byte-identical
rest vs focus, against a default control that differed. Cause: I had
moved the ring onto the indicator, so a replacement omitting it left
nothing to see — the real input is opacity: 0. Passing the style down
as a prop does not fix it either; the sample replacement in our own
Storybook destructures {state, size, isDisabled} and drops everything
else, which is what a theme author plausibly writes.
The ring goes back on the owner, where a replacement cannot reach it.
The cost is that a replacement of a different shape gets the built-in's
ring shape — a cosmetic bug in place of an accessibility failure. I
tried a CSS variable to hand the shape back, and dropped it: the
theming architecture requires every documented var to be reachable
through derived[] standard-property mapping, which would have hijacked
borderRadius on the whole component. Not worth a new theme surface for
a case nobody has hit.
BLOCK — the docs recommended --color-positive and --color-muted, which
do not exist, so following the restyle example produced an invisible
checkbox. This is precisely what the vibe test in #4831 measured
replacements doing (4/4 invented tokens) and what its docs
recommendation was meant to prevent. Every token named in the
directory now exists in tokens.stylex.ts, verified by grep.
Targets renamed to follow the component-name convention:
checkbox -> checkbox-indicator, radio -> radio-indicator, radio-dot ->
radio-indicator-dot. A target is public API, so the old names are still
emitted on the same element via a new themeProps legacyNames option;
themes styling them keep working until the next major. Verified in
Chrome that both selectors paint and a bogus one does not.
Also removes __audit_probe2.tsx, which my swept in from
the audit run.
Verified: cb-focus and radio-focus are 0px different from origin/main
(the ring renders exactly as it always did); the themed replacement now
shows focus; 876 tests pass across the touched components.
…ner guarantees Answers 'why can't the indicator control the outline and the owner control its display'. It can, and :has() is what makes it work. - The indicator draws its own ring, so it traces its own shape — the built-in checkbox rings a rounded square, the radio rings a circle, neither describing itself (outline follows border-radius). - The owner draws one only when the indicator doesn't. An indicator that takes over sets a marker; the owner's rule suppresses itself through ':has([data-indicator-focus]):has(:focus-visible)', which out-specifies its own ':has(:focus-visible)' two pseudo-classes to one. So a third-party replacement that omits the ring gets the owner's rather than nothing (WCAG 2.4.7), and one that wants a different shape takes it over — the compromise in the previous commit is gone, along with the owners' hardcoded guesses about the shape they host. The ring and the suppression marker ship as ONE spread (indicatorFocusRingProps()), because setting the marker without drawing a ring would be worse than either alone. Verified in Chrome, all three cases focus-visible: built-in checkbox indicator r=6px, owner stands down built-in radio indicator r=50%, owner stands down bare replacement owner r=6px, indicator draws none And 0px changed vs origin/main on cb-focus, radio-focus, and the variation sweeps — the rings look exactly as they do today. Mechanism proven in isolation first (a standalone :has() page), which is how I caught that a sibling combinator cannot see a preceding input: the indicator's ring has to key off the ancestor scope marker. Also fixes a silently-dropped 5th argument to mergeProps, which takes four positional arguments — the ring props are nested into the first instead.
…d outline Replaces the :has()-suppression machinery with the approach #4654 makes possible: the owner paints the standard ring onto the indicator's own element at focus time. Why the owner and not the indicator: the control's native input is opacity: 0, so the ring has to land on the picture beside it -- and that picture is third-party code once a theme replaces it. A replacement that simply does not draw one ships a control with no visible focus (WCAG 2.4.7), which is the DEFAULT outcome: our own sample replacement destructures {state, size, isDisabled} and drops the rest. Nothing can enforce otherwise; a component ignoring every prop is still assignable to IndicatorComponent, and branding the return type does not work either because JSX types every expression as JSX.Element. Why the indicator's element and not a wrapper: outline follows that element's border-radius, so the ring is shaped correctly without the owner hardcoding a guess about what it hosts. RadioListItem used to carry border-radius: 50% for a circle it did not own. Two details, both measured rather than assumed: - Inline style, not an injected class. React replaces className wholesale on re-render, so an injected class disappears the moment the control changes state -- pressing Space on a focused checkbox lost the ring. React reconciles style per property, so an outline set this way survives. - The handler checks :focus-visible. The CSS this replaces got keyboard-vs-pointer for free; asking the element keeps the browser's heuristic, including focus({focusVisible: true}). Adds FOCUS_OUTLINE_PARTS to the shared focus-outline module -- the same ring as its StyleX variants, exposed as longhand values for the one case that must apply it imperatively. Longhands rather than the outline shorthand for the reason #4654 established: a shorthand resets every longhand it covers, so a caller could not re-color the ring. Each owner wraps its indicator in a display: contents slot, so the ring has one unambiguous target whatever a theme renders. Verified in Chrome: built-in checkbox rings at r=6px, built-in radio at r=50%, a replacement that forwards nothing still rings, and a mouse click rings nothing. 298 tests pass across the touched components.
3283572 to
df2430d
Compare
Review cleanup on facebook#4628, applying the paint-vs-layout frame: - multi-selector-option is the only new target, and now carries the row's size plus its select-all / selected / disabled states, so a theme can express "selected option at large" or restyle just the Select All row. - Row typography moves from the label span onto the row, so a theme's font override on the row target actually reaches the text — and reaches renderOption content too, not just the fallback span. - Drops -option-checkbox: the wrapper is inert/layout-only, and the checkbox visual is directly themeable via checkbox-indicator (facebook#4712). - Drops -select-all (now a state) and -select-all-divider (a target whose purpose is hiding an internal element is a design question, not theming).
…try types The docsite keeps its own copy of the authoring doc types, and `next build` type-checks the emitted componentRegistry.ts against it. Two fields had drifted out of that copy, so any full docsite build failed: deprecatedFor added to ComponentThemingTarget by #4712 (Indicator's renamed theme targets) replaces on ComponentThemingDerivedVar, used by TextArea Pre-existing on main, not introduced here -- verified by building the docsite on a clean origin/main worktree, which fails with the same `'deprecatedFor' does not exist in type 'ThemingTarget'`, and passes (315/315 static pages) with only this one-file fix applied. It surfaced on this PR because the change touches core .doc.mjs inputs, so Vercel's ignored-build-step ran a docsite build for the first time since #4712. Adds SYNC breadcrumbs on both authoring interfaces, since `check:sync` is the mechanism the repo already uses to catch exactly this drift. Diffed all six shared shapes; these two were the only gaps.
…try types The docsite keeps its own copy of the authoring doc types, and `next build` type-checks the emitted componentRegistry.ts against it. Two fields had drifted out of that copy, so any full docsite build failed: deprecatedFor added to ComponentThemingTarget by #4712 (Indicator's renamed theme targets) replaces on ComponentThemingDerivedVar, used by TextArea Pre-existing on main, not introduced here -- verified by building the docsite on a clean origin/main worktree, which fails with the same `'deprecatedFor' does not exist in type 'ThemingTarget'`, and passes (315/315 static pages) with only this one-file fix applied. It surfaced on this PR because the change touches core .doc.mjs inputs, so Vercel's ignored-build-step ran a docsite build for the first time since #4712. Adds SYNC breadcrumbs on both authoring interfaces, since `check:sync` is the mechanism the repo already uses to catch exactly this drift. Diffed all six shared shapes; these two were the only gaps.
#4913) * fix(indicator): a falsy child no longer deletes the state mark The busy idiom a host writes is `children={isBusy && <Spinner/>}`, which passes `false` when it is not busy. `false` is not null and is not caught by `??`, so every indicator took its children path, rendered nothing in it, and dropped the checkmark, the checkbox tick and the radio dot on every selected row. Filed as #4893 against CheckIndicator, where #4890 moved the branch. It was wider: measured before writing anything, ALL THREE indicators lost their mark on `false` and on `''`. CheckIndicator additionally leaked an empty span. #4890 widened its case from one state to every state -- the unchecked early return had been masking it. All three now guard with `isRenderable`, the util the repo already ships for this. `0` still counts as content: it renders the character "0". Also, both from the #4890 audit and contained to the same lines: CheckIndicator's children slot now reserves the glyph's 1rem box and carries its color, so swapping a Spinner in neither shifts the row nor loses the disabled shade. Adds apps/storybook/stories/Indicator.stories.tsx -- the indicators had no story file (#4894), which is why the children path had no rendered coverage and this shipped unseen. Not fixed here: `ref` is dropped on the glyph path. Icon in string mode routes through IconFromRegistry, which takes no ref, so that is a change to Icon's ref behavior for every caller -- its own PR. * fix(storybook): key the Indicator stories to the component, not to meta CI caught what my local run could not: 'satisfies Meta' + StoryObj<typeof meta> makes `args` required on every story, and `state` is required on an indicator, so five render-only stories failed the storybook typecheck. 122 of the 123 story files use `const meta: Meta<typeof X>` with `StoryObj<typeof X>`; mine was the one that did not. Now it matches. Verified the way CI does, after a full `pnpm build`: storybook typecheck clean, storybook build completes, core typecheck (incl. tests) clean, cli typecheck:strict clean, docsite generate + 364 tests green. * fix(docsite): carry deprecatedFor and replaces in the generated registry types The docsite keeps its own copy of the authoring doc types, and `next build` type-checks the emitted componentRegistry.ts against it. Two fields had drifted out of that copy, so any full docsite build failed: deprecatedFor added to ComponentThemingTarget by #4712 (Indicator's renamed theme targets) replaces on ComponentThemingDerivedVar, used by TextArea Pre-existing on main, not introduced here -- verified by building the docsite on a clean origin/main worktree, which fails with the same `'deprecatedFor' does not exist in type 'ThemingTarget'`, and passes (315/315 static pages) with only this one-file fix applied. It surfaced on this PR because the change touches core .doc.mjs inputs, so Vercel's ignored-build-step ran a docsite build for the first time since #4712. Adds SYNC breadcrumbs on both authoring interfaces, since `check:sync` is the mechanism the repo already uses to catch exactly this drift. Diffed all six shared shapes; these two were the only gaps.
…d size (#4628) * feat(multi-selector): expose dropdown row/checkbox/select-all theme targets Expose theme targets for the dropdown option rows, per-row checkbox, Select All row, and Select All divider. Font styling is themeable via the option target directly (no separate label target needed). The option target is intended for visual overrides (borderRadius, backgroundColor, padding) — the layout properties (flex, gap) are internal structure and should not be themed. * feat(multi-selector): one themeable option target, with states and size Review cleanup on #4628, applying the paint-vs-layout frame: - multi-selector-option is the only new target, and now carries the row's size plus its select-all / selected / disabled states, so a theme can express "selected option at large" or restyle just the Select All row. - Row typography moves from the label span onto the row, so a theme's font override on the row target actually reaches the text — and reaches renderOption content too, not just the fallback span. - Drops -option-checkbox: the wrapper is inert/layout-only, and the checkbox visual is directly themeable via checkbox-indicator (#4712). - Drops -select-all (now a state) and -select-all-divider (a target whose purpose is hiding an internal element is a design question, not theming). --------- Co-authored-by: athz <28840834+athz@users.noreply.github.com> Co-authored-by: Cindy Zhang <cindyxz@meta.com>
…ets guard MoreMenu hand-assembled its `astryx-more-menu` theme target with `stableClassName()` and string concatenation. Route it through `themeProps()`/`mergeProps()` instead, so the target sits on the same reflection surface as every other one. The rendered DOM is unchanged. themingTargets.test.ts grows the second direction of the guard — documented targets must actually be rendered — and scans both `@astryxdesign/core` and `@astryxdesign/lab` package-wide for the `docs` block. `docsZh` stays a per-directory check. Rebased onto main, which moved 69 commits under it: - `themeProps()` gained a third `options.legacyNames` argument (#4712, #4890) for targets renamed behind a deprecation window. The extractor only read the first two arguments, so `astryx-checkbox`, `astryx-radio` and `astryx-radio-dot` — emitted through `legacyNames` and deliberately documented with `deprecatedFor` — looked unrendered. Teach it to read `legacyNames` string literals as rendered sites of their own, with the same prop keys (they land on the same element), and cover it with two extractor tests. - ListInput (#4740) and TransferList (#4870) landed after this branch was cut and render five undocumented targets between them. Document them, the same way the other lab components here are documented. - Stepper's targets were extended on main with the same `indicatorPosition` entry this branch added, plus `astryx-step-indicator` and the matching `docsZh` block; kept main's superset. Both sides' new MoreMenu tests are kept.
…ets guard MoreMenu hand-assembled its `astryx-more-menu` theme target with `stableClassName()` and string concatenation. Route it through `themeProps()`/`mergeProps()` instead, so the target sits on the same reflection surface as every other one. The rendered DOM is unchanged. themingTargets.test.ts grows the second direction of the guard — documented targets must actually be rendered — and scans both `@astryxdesign/core` and `@astryxdesign/lab` package-wide for the `docs` block. `docsZh` stays a per-directory check. Rebased onto main, which moved 69 commits under it: - `themeProps()` gained a third `options.legacyNames` argument (#4712, #4890) for targets renamed behind a deprecation window. The extractor only read the first two arguments, so `astryx-checkbox`, `astryx-radio` and `astryx-radio-dot` — emitted through `legacyNames` and deliberately documented with `deprecatedFor` — looked unrendered. Teach it to read `legacyNames` string literals as rendered sites of their own, with the same prop keys (they land on the same element), and cover it with two extractor tests. - ListInput (#4740) and TransferList (#4870) landed after this branch was cut and render five undocumented targets between them. Document them, the same way the other lab components here are documented. - Stepper's targets were extended on main with the same `indicatorPosition` entry this branch added, plus `astryx-step-indicator` and the matching `docsZh` block; kept main's superset. Both sides' new MoreMenu tests are kept.
…ets guard MoreMenu hand-assembled its `astryx-more-menu` theme target with `stableClassName()` and string concatenation. Route it through `themeProps()`/`mergeProps()` instead, so the target sits on the same reflection surface as every other one. The rendered DOM is unchanged. themingTargets.test.ts grows the second direction of the guard — documented targets must actually be rendered — and scans both `@astryxdesign/core` and `@astryxdesign/lab` package-wide for the `docs` block. `docsZh` stays a per-directory check. Rebased onto main, which moved 69 commits under it: - `themeProps()` gained a third `options.legacyNames` argument (#4712, #4890) for targets renamed behind a deprecation window. The extractor only read the first two arguments, so `astryx-checkbox`, `astryx-radio` and `astryx-radio-dot` — emitted through `legacyNames` and deliberately documented with `deprecatedFor` — looked unrendered. Teach it to read `legacyNames` string literals as rendered sites of their own, with the same prop keys (they land on the same element), and cover it with two extractor tests. - ListInput (#4740) and TransferList (#4870) landed after this branch was cut and render five undocumented targets between them. Document them, the same way the other lab components here are documented. - Stepper's targets were extended on main with the same `indicatorPosition` entry this branch added, plus `astryx-step-indicator` and the matching `docsZh` block; kept main's superset. Both sides' new MoreMenu tests are kept.
…ets guard MoreMenu hand-assembled its `astryx-more-menu` theme target with `stableClassName()` and string concatenation. Route it through `themeProps()`/`mergeProps()` instead, so the target sits on the same reflection surface as every other one. The rendered DOM is unchanged. themingTargets.test.ts grows the second direction of the guard — documented targets must actually be rendered — and scans both `@astryxdesign/core` and `@astryxdesign/lab` package-wide for the `docs` block. `docsZh` stays a per-directory check. Rebased onto main, which moved 69 commits under it: - `themeProps()` gained a third `options.legacyNames` argument (#4712, #4890) for targets renamed behind a deprecation window. The extractor only read the first two arguments, so `astryx-checkbox`, `astryx-radio` and `astryx-radio-dot` — emitted through `legacyNames` and deliberately documented with `deprecatedFor` — looked unrendered. Teach it to read `legacyNames` string literals as rendered sites of their own, with the same prop keys (they land on the same element), and cover it with two extractor tests. - ListInput (#4740) and TransferList (#4870) landed after this branch was cut and render five undocumented targets between them. Document them, the same way the other lab components here are documented. - Stepper's targets were extended on main with the same `indicatorPosition` entry this branch added, plus `astryx-step-indicator` and the matching `docsZh` block; kept main's superset. Both sides' new MoreMenu tests are kept.
…ets guard MoreMenu hand-assembled its `astryx-more-menu` theme target with `stableClassName()` and string concatenation. Route it through `themeProps()`/`mergeProps()` instead, so the target sits on the same reflection surface as every other one. The rendered DOM is unchanged. themingTargets.test.ts grows the second direction of the guard — documented targets must actually be rendered — and scans both `@astryxdesign/core` and `@astryxdesign/lab` package-wide for the `docs` block. `docsZh` stays a per-directory check. Rebased onto main, which moved 69 commits under it: - `themeProps()` gained a third `options.legacyNames` argument (#4712, #4890) for targets renamed behind a deprecation window. The extractor only read the first two arguments, so `astryx-checkbox`, `astryx-radio` and `astryx-radio-dot` — emitted through `legacyNames` and deliberately documented with `deprecatedFor` — looked unrendered. Teach it to read `legacyNames` string literals as rendered sites of their own, with the same prop keys (they land on the same element), and cover it with two extractor tests. - ListInput (#4740) and TransferList (#4870) landed after this branch was cut and render five undocumented targets between them. Document them, the same way the other lab components here are documented. - Stepper's targets were extended on main with the same `indicatorPosition` entry this branch added, plus `astryx-step-indicator` and the matching `docsZh` block; kept main's superset. Both sides' new MoreMenu tests are kept.
1 participant
Adds indicators — the componentized form of stateful control visuals — and makes them replaceable by name from a theme.
What an indicator is
The checkbox box, the radio circle, the mark on a chosen option. The owning component keeps the input, role, accessible name, focus, and keyboard behavior; the indicator turns state into a picture. It renders
aria-hiddenand owns no semantics.Consumer-facing API
Replace an indicator. Replacement is by name, so one entry reaches every component that draws it:
Every single-selection mark in the app is now a radio — Selector options included, drawing an empty circle on unselected rows, which a check mark never could.
Restyle one instead. An indicator renders stable
astryx-*targets, sodefineTheme({components: {checkbox: …}})reaches it like any other component. Prefer this; replacement is for when the shape itself is wrong.Write a replacement. Four rules, each one a mistake made by hand in this PR's first draft:
childrenwhen present (the owner passes a loading Spinner through it)aria-hidden(the owner provides role and accessible name)Families
A family fixes the state space, and each uses the ARIA vocabulary for the state it expresses:
singleSelectionunchecked|checkedaria-checkedcheck,radiomultiSelectionunchecked|checked|indeterminatearia-checkedcheckboxThe registry is family-checked, so a replacement can't be written against the wrong states — a
radioreplacement expectingindeterminateis a type error. Adding a family is additive and module-augmentable.Vocabulary comes from the vibe test in #4831, not from taste: given one shared enum across families, generators re-derived the split anyway and wrote
aria-expanded={isOpen}besidestate="on"on adjacent lines.Notable
state.{isSelected && …}would make a replaced radio unable to draw its unselected form. The default check still renders nothing when unchecked.CheckIndicatorrenders<Icon>directly rather than wrapping it, so a host's theme target lands on the glyph — and it adds no new theme target, sinceastryx-iconplus the host's target already reach it. Continues the wrapper reduction from fix(core): drop styling wrappers around icons; render glyphs through Icon #4838/fix(selector): drop the shared trigger-icon wrapper #4846.CheckboxInput's input isopacity: 0, so with the ring inside the indicator, a theme replacement left the control with no visible focus (WCAG 2.4.7). Moved to the wrapper that owns the input, mirroringRadioListItem. Found by an independent review pass; pinned by tests.prefers-reduced-motionoverride it lacked, and menu radios gain forced-colors treatment.Migration
Menu radios now use the shared
radio/radio-dottargets:dropdown-menu-radio-dotis removed — targetradio-dot, which now also reaches RadioList.dropdown-menu-radiotoradio; layout rules (size, order) stay.astryx theme buildreports the removed key. Runtime themes are not validated.Test plan
Indicator.test.tsx— contract per indicator, and family typing.Selector.test.tsx— the default mark is the glyph itself (no wrapper); a theme mappingcheck→RadioIndicatorputs a radio on every row and fills exactly the selected one.CheckboxInput.test.tsx— focus ring survives indicator replacement. Verified by reverting the fix and confirming failure.origin/mainon the same quiet machine (Table perf budget, TransferList draft timing — pre-existing flakes). 6528 passing vs main's 6516.CI=true(strict mode — warnings become errors).Visual verification with before/after screenshots to follow.
Closes #4831