[CDX-XXX] Add opt-in radio selection mode to FilterOption component - #55
[CDX-XXX] Add opt-in radio selection mode to FilterOption component#55niizom wants to merge 1 commit into
Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
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 appropriate tests and Storybook stories — the implementation is clean and well-scoped.
Inline comments: 5 discussions added
Overall Assessment:
| /** | ||
| * Selection input type. | ||
| * @default 'checkbox' | ||
| * @deprecated The checkbox default for single-selection facets will be deprecated |
There was a problem hiding this comment.
Important Issue: The @deprecated tag on selectionType is misleading — the prop itself is not being deprecated, only the default value behaviour is changing in a future major version. Using @deprecated on a prop signals to consumers that the entire prop should be avoided, which is incorrect here. Remove the @deprecated tag and instead document the future default change in a @remarks or plain prose note:
/**
* Selection input type.
* @default 'checkbox'
* Note: In the next major version the default will change to 'radio' for single-selection facets.
*/
selectionType?: 'checkbox' | 'radio';There was a problem hiding this comment.
This is direct requirement from a ticket:
Add a deprecation notice on the prop / current default: "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."
| * 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. */ |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| const checkboxEl = checkboxVisible && ( | ||
| const indicatorVisible = checkboxPosition !== 'none'; | ||
|
|
||
| const checkboxEl = ( |
There was a problem hiding this comment.
Suggestion: checkboxEl and radioEl are defined unconditionally on every render even when only one (or neither) will ever be used. Since they are plain JSX with no conditional logic, this is low-cost, but it does mean React creates VDOM nodes that are immediately discarded when indicatorVisible is false or a different selectionType is active. Moving the definitions inside the indicatorEl expression (or extracting them as small sub-components memoised with useMemo) would make intent clearer and avoids unnecessary allocations:
const indicatorEl = indicatorVisible && (
selectionType === 'radio' ? (
<div className='cio-radio ...'> ... </div>
) : (
<div className='cio-checkbox ...'> ... </div>
)
);| expect(document.querySelector('.cio-checkbox')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('calls onChange and respects checkboxPosition for radio', () => { |
There was a problem hiding this comment.
Suggestion: This test name conflates two unrelated concerns (onChange callback and layout position). Splitting it into two focused tests would improve readability and failure diagnostics. Additionally, the test description says "calls onChange" but the layout assertion (displayDiv?.nextElementSibling) is the more novel/important part — if the layout assertion fails the test name gives no hint why.
| expect(radio).not.toBeChecked(); | ||
| }); | ||
|
|
||
| test('renders radio visual indicator and responds to checked state', () => { |
There was a problem hiding this comment.
Suggestion: There is no test covering the selectionType='radio' with checkboxPosition='left' (the default) to confirm the indicator appears to the left. The existing position test only verifies checkboxPosition='right'. Adding a symmetric test for the left position would give complete coverage of the checkboxPosition + selectionType='radio' combination.
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; |
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?