Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner providing or identifying the governing spec for SelectableCard's new stable inputType/name props and the lab SelectableCardGroup built on them: no current SelectableCard record covers them, so what must be settled is whether a single-choice card set becomes a native radio group through these props, recorded in a SelectableCard component record that names the adopted radio-group parts (AST-002 DEC-5, AST-020 FR15). An engineering owner records that decision by commenting /approve-spec b7e0cb67d92da148bfb7838034529f00d0350f78 on this pull request, and the author may do so if they are an engineering owner. The radio behavior itself checks out in Chromium (one tab stop on the chosen card, arrow keys that move and select), and these fixes are needed whatever the decision:
- A disabled card set still submits
<name>=onwith its form, so detach the input while disabled (form="", asRadioListItemdoes) to meet AST-085 FR3. SelectableCardGroupcomposesField, so input-fields FR10 asks forcontainer*styling props in place of rootxstyle/className/style.- The native input name should be
htmlName, the prop RadioList, CheckboxInput and the other core inputs use, rather thanname. pnpm -F @astryxdesign/lab typecheck:docsfails becausedocsZhinSelectableCardGroup.doc.mjshas nousageblock.
[Automated review]
| ref={interactiveRef} | ||
| type="checkbox" | ||
| type={isRadio ? 'radio' : 'checkbox'} | ||
| name={name} |
There was a problem hiding this comment.
A disabled card keeps this name, so a checked disabled card still submits <name>=on with its form (AST-085 FR3: a disabled field contributes no value). Detaching the input while disabled keeps the radio grouping: form={isDisabled ? '' : undefined}, as RadioListItem does.
| /** | ||
| * The native input's `name`. Radio cards that share it form one group. | ||
| */ | ||
| name?: string; |
There was a problem hiding this comment.
Every core input that forwards the native input name calls it htmlName (RadioList, CheckboxInput, Switch, TextInput and others). Calling it name here gives the same pass-through a second name (public-component-api INV2), and renaming after release would be breaking.
| onChange: (value: string[]) => void; | ||
| } | ||
|
|
||
| export type SelectableCardGroupProps = Omit<BaseProps, 'onChange'> & |
There was a problem hiding this comment.
This component composes Field, so input-fields FR10 applies: no root xstyle/className/style (a type error), and containerXstyle/containerClassName/containerStyle reach the Field box. Omit<BaseProps, 'onChange' | 'xstyle' | 'className' | 'style'> plus container* props would conform.
| }; | ||
|
|
||
| /** @type {import('@astryxdesign/cli/authoring').ComponentDoc} */ | ||
| export const docsZh = { |
There was a problem hiding this comment.
docsZh is typed as ComponentDoc but has no usage, so pnpm -F @astryxdesign/lab typecheck:docs, and with it CI's typecheck job, fails with TS2322. Add a translated usage block, as the docsZh of InfoTip and Stat do, or type it as ComponentTranslationDoc.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNew ComponentsSelectableCardGroup (@astryxdesign/lab) · View in Storybook
Modified ComponentsSelectableCard (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: 2 accessibility violation(s) found — 2 serious. Checkbox pattern - 1 issue(s)
SelectableCard - 1 issue(s)
Visual Regression48 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 28 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…electableCardGroup) SelectableCard takes inputType="radio" and a name: its hidden input becomes a native radio in that named group, so the set is one tab stop, the arrow keys move and select with wrapping, and the position is spoken. A radio card selects and never deselects itself; Enter selects as Space does. The default stays checkbox. The lab SelectableCardGroup wraps a set: role="radiogroup" named and described by its Field label, with value/onChange for one value, or a named group of checkboxes with selectionMode="multiple".
…s htmlName, the group styles its Field box - A disabled card kept its input in the form, so a checked disabled card submitted `<name>=on` (spec:AST-085 FR3). Its input is now detached while disabled (`form=""`, as RadioListItem does); the name keeps the radio grouping. - The native name prop is `htmlName`, as on RadioList, CheckboxInput and the other core inputs (public-component-api INV2), on SelectableCard and on the lab SelectableCardGroup. - SelectableCardGroup composes Field, so it takes `containerXstyle`, `containerClassName` and `containerStyle` for the Field box and no root xstyle, className or style (input-fields FR10). - The group's Chinese doc has its `usage` block, so the lab's `typecheck:docs` passes.
There was a problem hiding this comment.
Approval waits on an engineering owner deciding SelectableCard's new stable inputType/htmlName props and the lab SelectableCardGroup built on them: no current SelectableCard record covers them, so what must be settled is whether a single-choice card set becomes a native radio group through these props, recorded in a SelectableCard component record that names the adopted radio-group parts (AST-002 DEC-5, AST-020 FR15). An engineering owner records that decision by commenting /approve-spec 96e402d37dc2977b1fbfbb5b97713afac423184f on this pull request, and the author may do so as an engineering owner. The earlier disabled-submission, styling, naming and docs-typecheck points are fixed and the radio keyboard behaves as described in Chromium; these points remain whatever the decision:
- Inside a
<form>,form=""gives a per-card disabled radio a different form owner from its siblings, so it becomes a second tab stop and ArrowRight skips it (AST-085 FR1); keeping the whole set on one form owner fixes it. - A form receives
<htmlName>=onfor every card, although the test "an enabled group submits its choice under htmlName" and the docs suggest the choice is submitted. SelectableCardGroupdrops root styling and appliescontainer*in every layout without the two development warnings input-fields FR10 asks for.SelectableCard.doc.mjsstill tells callers to give the set onename, and that file now conflicts withmain(#8057), so a rebase is needed.
[Automated review]
| // Detached from its form while disabled, so it neither submits nor | ||
| // blocks submission (spec:AST-085 FR3); the name keeps the radio | ||
| // grouping. | ||
| form={isDisabled ? '' : undefined} |
There was a problem hiding this comment.
Inside a <form>, form="" gives a per-card disabled radio no form owner while its siblings keep the form, and HTML groups radios by name and form owner. In Chromium a set with Pro disabled then takes two tab stops (Tab from Basic lands on Pro) and ArrowRight from Basic skips Pro, against AST-085 FR1 (a disabled control keeps the focus order it has when enabled). Keeping the whole set on one form owner preserves the grouping, for example by having the group detach every radio and submit the chosen value through one hidden input.
| name: 'inputType', | ||
| type: "'checkbox' | 'radio'", | ||
| description: | ||
| 'The native control inside the card. A radio card is one choice of a set: it selects and never deselects itself. Give the set one `name` and put it in a role="radiogroup" with an accessible name; the browser then makes it one tab stop, arrow keys move and select, and "2 of 3" is spoken.', |
There was a problem hiding this comment.
This still says name; the prop is htmlName. An untyped name never reaches the input, so each card would be a group of one.
| if (form == null) { | ||
| throw new Error('no form'); | ||
| } | ||
| expect([...new FormData(form).entries()]).toEqual([['plan', 'on']]); |
There was a problem hiding this comment.
This asserts on, so a form cannot tell which card was chosen, while the test title says the group submits its choice. RadioListItem passes its value to the input; either carry the card's value, or retitle the test and say in the docs that htmlName only groups the radios.
| const descriptionID = useId(); | ||
| const generatedName = useId(); | ||
| const name = htmlName ?? generatedName; | ||
| // An untyped caller's root styling never reaches the group element. |
There was a problem hiding this comment.
input-fields FR10 also asks for a development warning when an untyped caller passes root xstyle/className/style, and when container* is passed where no Field box renders (a horizontal-labels FormLayout or InputGroup). Core's useFieldInputStyling does both for every core Field-composed input; useDevWarning from @astryxdesign/core/hooks would cover it here.
96e402d to
9db2b39
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding SelectableCard's new stable inputType/htmlName props and the lab SelectableCardGroup built on them. No current SelectableCard record covers them, so what must be settled is whether a single-choice card set becomes a native radio group through these props, recorded in a SelectableCard component record that names the adopted radio-group parts (AST-002 DEC-5, AST-020 FR15). An engineering owner records that decision by commenting /approve-spec 9db2b390dba681804c6b0390c5c4959bbbf36545 on this pull request, and the author may do so as an engineering owner. The radio keyboard behaves as described in Chromium, and these points remain whatever the decision:
- Inside a
<form>,form=""gives a card disabled on its own a different form owner from its siblings, so it becomes a second tab stop and ArrowRight skips it (AST-085 FR1); keeping the whole set on one form owner fixes it. - A form receives
<htmlName>=onfor every card, although the test "an enabled group submits its choice under htmlName" and the docs ("ashtmlNameon RadioList") say the choice is submitted. SelectableCardGroupdrops untyped root styling and forwardscontainer*in every layout without the two development warnings input-fields FR10 asks for.SelectableCard.doc.mjsstill tells callers to give the set onename, but the prop ishtmlName.
[Automated review]
# Conflicts: # .github/workflows/ci.yml # packages/core/src/SelectableCard/SelectableCard.doc.mjs # packages/core/src/SelectableCard/SelectableCard.tsx
There was a problem hiding this comment.
Approval waits on an engineering owner deciding SelectableCard's new stable inputType/htmlName props and the lab SelectableCardGroup built on them: no current SelectableCard record covers them, so what must be settled is whether a single-choice card set becomes a native radio group through these props, recorded in a SelectableCard component record that names the adopted radio-group parts (AST-002 DEC-5, AST-020 FR15). An engineering owner records that decision by commenting /approve-spec dbb7db18331d1a0cac190cb943e9accd5aac25b3 on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these points need fixing:
- Inside a
<form>,form=""gives a card disabled on its own a different form owner from its siblings, so it becomes a second tab stop and ArrowRight skips it (AST-085 FR1). - A form receives
<htmlName>=onfor every card, while the test "an enabled group submits its choice under htmlName" and the docs' comparison with RadioList suggest the choice is submitted. SelectableCard.doc.mjsstill says not to use the card for single-select, next to the newinputType: 'radio'entry.SelectableCardGroupreplaces a caller'saria-labelledby/aria-describedby, drops untyped root styling silently, and forwardscontainer*in every layout, where input-fields FR10 asks it to keep the caller's ids and to warn in development.
[Automated review]
| {name: 'isSelected', type: 'boolean', description: 'Controlled selection state.', required: true}, | ||
| {name: 'onChange', type: '(isSelected: boolean) => void', description: 'Called when toggled.', required: true}, | ||
| {name: 'onChange', type: '(isSelected: boolean) => void', description: 'Called when toggled. A radio card only calls it with true.', required: true}, | ||
| {name: 'inputType', type: "'checkbox' | 'radio'", description: "The native control inside the card. 'radio' makes the card one choice of a single-select set; SelectableCardGroup (lab) builds that set and documents it.", default: "'checkbox'"}, |
There was a problem hiding this comment.
This entry makes a radio card "one choice of a single-select set", but the same doc still lists "Use for a single-select group: each card is a checkbox…" as a Don't (lines 17 and 75) and describes a card that toggles (lines 11 and 15); astryx component SelectableCard --dense prints both on one page. If the props are accepted, replace the Don't with when to use inputType="radio" or SelectableCardGroup, and say that a radio card selects without toggling off, in docs and docsDense.
Review only this PR's change: vjeux/astryx@8c3f3be...dbb7db1.
User need
A plan picker built from SelectableCards is a single choice, but each card is a checkbox: no position is spoken, every card is a tab stop, and arrows do nothing. A radio group has one tab stop, arrows that move and select, and "2 of 3" spoken.
Evidence
iOS 26.2 Mobile Safari: a tap moves the choice from Pro to Team.
Keyboard, Chromium (captions read the page's focus and selection). Before: 3 checkboxes, 3 tab stops. After: 3 radios, 1 tab stop, arrows move the choice and wrap:
Problem and solution fit
inputType="radio"and anhtmlName, as RadioList does. Its hidden input becomes a radio in that group, so the browser supplies the tab stop, arrow keys and position. A radio card never deselects itself; Enter selects as Space does. A disabled card submits nothing (AST-085 FR3).role="radiogroup"named by a visible Field label, withvalue/onChange;container*styles its Field box (input-fields FR10).selectionMode="multiple"gives a namedgroupof checkboxes.Current specification
architecture:public-component-api(current): prop conventions. Proposed; pending owner review:inputTypeandhtmlNameon SelectableCard (into itsexperimentalobject first if owners prefer, AST-017 FR21).Public delta and caller burden
checkboxdefault leaves existing cards unchanged.SelectableCardGroupandSelectableCardGroupItem. Callers pass a value per card and own the state.Design and alternatives
role="radio"on the card div (duplicating the input's focus and keyboard).RadioListItem,CheckboxListItem, bound through the list's context); a card is its own surface with free content, often in a grid, so hosting cards would add a second item anatomy.inputType+htmlName) is deprecated in a follow-up (AST-017 lifecycle;architecture:public-component-apiINV9).Scope
Testing
Main 8c3f3be (with #8419) merged in; the results below are from the merged head.
pnpm vitest runSelectableCard, lab SelectableCardGroup and Card: 44 pass;build-css12/12. New: a disabled group's form data is empty while its radios keep one name; an enabled group submits its choice underhtmlName;container*styles the Field box, not the group (all three failed on the previous head).SelectableCardRadio.test.tsx: a radio card is a native radio in the named group, a click selects and unchecks the others, the chosen card never deselects (click or Enter), Enter selects, a disabled radio card stays focusable and selects nothing, the default stays a checkbox. 5 of 6 fail without the change.SelectableCardGroup.test.tsx: a named, described radiogroup of radios sharing one name; choosing moves the value; the chosen card again changes nothing; a disabled group;multipleis a named group of checkboxes that toggle; an item outside a group throws. Full lab suite: 840 pass.pnpm test:a11y-contract-style Chromium specSelectableCardRadio.a11y.chromium.spec.ts(found bya11y-pr-contracts.mjs, scoped to SelectableCard; no workflow step): 3/3. It checks the named radiogroup of native radios, one tab stop on the checked card (Tab out, Shift+Tab back), Right/Left moving and selecting with wrapping, Space, and a click that keeps the choice. The CI routing, shard and scope tests pass (45).typecheck:docspasses (it failed before the second commit: the Chinese doc had nousage).pnpm build; the ShadCN registry test (29/29, route lock included); the whole CLI node suite run one file at a time (9514 pass, 3 skipped; in parallel on a loaded box some files hit worker errors, and each passes alone);check-changesets; core typecheck; lab typecheck except one error that is also on main (MobileTokenizer.test.tsx); eslint, prettier.