[CDX-484] Add opt-in radio selection mode to FilterOption component - #55
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in radio-button selection mode to the FilterOption UI component (and its visual variant), enabling single-select facet UX while preserving the existing checkbox default behavior.
Changes:
- Introduces
selectionType('checkbox' | 'radio') andgroupNameprops toFilterOption, and renders a radio-style indicator when opted in. - Adds Storybook stories demonstrating radio selection for both
FilterOptionandFilterOptionVisual. - Adds unit tests covering radio rendering/attributes and indicator behavior for both components.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stories/components/FilterOptionVisual/FilterOptionVisual.stories.tsx | Adds radio selection stories/controls for the visual filter option variant. |
| src/stories/components/FilterOption/FilterOption.stories.tsx | Adds radio selection stories/controls for FilterOption. |
| src/components/filter-option.tsx | Implements selectionType + groupName and renders a radio indicator when selected. |
| spec/components/FilterOptionVisual/FilterOptionVisual.test.tsx | Adds a test validating radio rendering and visual swatch presence. |
| spec/components/FilterOption/FilterOption.test.tsx | Adds tests validating radio attributes, indicator rendering, and positioning behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** | ||
| * Selection input type. | ||
| * @default 'checkbox' | ||
| * @deprecated The checkbox default for single-selection facets will be deprecated | ||
| * in the next major version — radio buttons will become the default for single-type facets. | ||
| */ | ||
| selectionType?: 'checkbox' | 'radio'; |
| /** Group name for the input. Required for radio inputs to form a radio group. */ | ||
| groupName?: string; |
6088af7 to
fc744d8
Compare
There was a problem hiding this comment.
Code Review
This PR adds an opt-in selectionType='radio' mode to FilterOption (and by extension FilterOptionVisual), with a custom radio indicator, groupName prop, and full test/story coverage — the implementation is clean and well-scoped.
Inline comments: 4 discussions added
Overall Assessment:
|
|
||
| const radioEl = ( | ||
| <div className='cio-radio cio:flex cio:justify-center cio:items-center cio:cursor-pointer cio:mx-2 cio:bg-white cio:w-5 cio:h-5 cio:min-w-5 cio:min-h-5 cio:rounded-full cio:transition-all cio:duration-250 cio:border cio:border-black/20'> | ||
| <div className='cio-radio-dot cio:w-2.5 cio:h-2.5 cio:rounded-full cio:bg-black cio:opacity-0 cio:transition-opacity cio:duration-250 cio:group-has-[input:checked]:opacity-100' /> |
There was a problem hiding this comment.
Important Issue: The cio:group-has-[input:checked] selector targets any checked input inside the group (the <li>), regardless of input type. With the checkbox variant this was fine because there was only one input type, but with radio inputs the same Tailwind variant is reused unchanged. In a real facet list every <li> is its own group, so a checked radio in one item could theoretically affect a different item's indicator if the nesting/group boundary is incorrect. More importantly, group-has-[input[type=radio]:checked] would be semantically more precise and defensive — consider scoping the selector to the specific input type for both the checkbox and radio indicators to prevent any cross-type leakage.
There was a problem hiding this comment.
Each <li> is its own group boundary, so the selector is already scoped to the single input within that item. There's no scenario where both a checkbox and radio coexist in the same group, so cross-type leakage isn't possible here.
| * in the next major version — radio buttons will become the default for single-type facets. | ||
| */ | ||
| selectionType?: 'checkbox' | 'radio'; | ||
| /** Group name for the input. Required for radio inputs to form a radio group. */ |
There was a problem hiding this comment.
Important Issue: groupName is documented as required for radio inputs but its TypeScript type is string | undefined (optional). There is no runtime warning or validation when selectionType='radio' is used without groupName. Without a name attribute, radio buttons in different <li> elements will not form a group and will not mutually exclude each other, silently producing broken UX. Add a console.warn (or use a discriminated union type) so consumers are alerted: e.g.
if (selectionType === 'radio' && !groupName) {
console.warn('[FilterOption] `groupName` is required when `selectionType` is "radio".');
}| const checkboxEl = checkboxVisible && ( | ||
| const indicatorVisible = checkboxPosition !== 'none'; | ||
|
|
||
| const checkboxEl = ( |
There was a problem hiding this comment.
Suggestion: Both checkboxEl and radioEl are always constructed unconditionally, even though only one of them (or neither, when checkboxPosition='none') is ever rendered. For a pure presentational component this is a negligible cost, but it also means the two JSX elements are recreated on every render regardless of selectionType. Consider moving the JSX inline into indicatorEl, or only constructing the element that matches the current selectionType:
const indicatorEl = indicatorVisible
? selectionType === 'radio'
? <div className='cio-radio ...'>...</div>
: <div className='cio-checkbox ...'>...</div>
: false;There was a problem hiding this comment.
The cost of constructing an unused JSX element (a plain div) is negligible as noted. The current separate variables are easier to read and maintain than nested ternaries.
| }); | ||
|
|
||
| describe('radio selection type', () => { | ||
| test('renders radio input with correct attributes and group name', () => { |
There was a problem hiding this comment.
Suggestion: There is no test covering the case where selectionType='radio' is used without groupName. Given that the JSDoc marks groupName as required for radio, a test verifying the rendered name attribute is absent (or a warning is emitted) would document the expected behaviour and guard against regressions. Similarly, no test exercises the interaction between multiple radio options in the same group to confirm mutual-exclusion semantics are honoured at the component API level.
Pull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?