New radio group primitive #1222
Conversation
WalkthroughThis change refactors the RadioGroupPrimitive component suite by replacing native radio inputs with a button-based approach, introducing a context-driven indicator, updating accessibility attributes, and restructuring the component hierarchy. It also updates related stories and tests to align with the new structure and usage patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Label
participant RadioGroupPrimitive.Item
participant RadioGroupPrimitive.Context
participant RadioGroupPrimitive.Indicator
User->>Label: Clicks label text
Label->>RadioGroupPrimitive.Item: Activates button
RadioGroupPrimitive.Item->>RadioGroupPrimitive.Context: Updates selection state
RadioGroupPrimitive.Context-->>RadioGroupPrimitive.Indicator: Provides itemSelected state
RadioGroupPrimitive.Indicator-->>User: Renders indicator if selected
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (1)
37-37: Replace non-null assertion with proper null checkingSame issue as above - avoid using non-null assertion operator.
🧹 Nitpick comments (1)
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (1)
22-22: Replace non-null assertion with proper null checkingUsing the non-null assertion operator (!) can hide potential runtime errors if the element is not found. Consider adding proper null checks or using
userEventfor better error messages.- fireEvent.click(radioA!); + if (radioA) fireEvent.click(radioA);Alternatively, use
@testing-library/user-eventfor better error handling:import userEvent from '@testing-library/user-event'; // ... await userEvent.click(radioA);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(2 hunks)src/core/primitives/RadioGroup/context/RadioGroupPrimitiveItemContext.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveIndicator.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(1 hunks)src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (2)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/core/primitives/RadioGroup/context/RadioGroupPrimitiveItemContext.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveIndicator.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🧬 Code Graph Analysis (1)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(3-7)
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 35-35: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 84-84: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (2)
5-5: Clean import addition.The import follows the established pattern for fragment components.
19-19: Consistent static property pattern.The Indicator property follows the same pattern as Root and Item, maintaining consistency in the API design.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
36-48: Excellent structural refactoring for button-based approach.The changes properly maintain accessibility and form compatibility:
- The
Primitive.divwrapper withrole="radiogroup"provides proper semantic structure- The hidden
inputelement ensures form submission works correctly- Accessibility attributes are correctly placed on the radiogroup container
- The hidden input properly reflects the current state (selectedValue, groupDisabled, required)
This approach allows for custom button-based radio items while maintaining standard form behavior.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
36-39: Good demonstration of new component structure.The story correctly shows the new pattern with:
- Label wrapping the RadioGroupPrimitive.Item
- Explicit styling classes on the item
- Indicator component nested inside for visual feedback
This provides clear usage examples for the refactored API.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveIndicator.tsx (1)
10-15: Well-implemented conditional indicator component.The implementation is clean and follows established patterns:
- Proper use of context for state management
- Efficient early return when not selected
- Consistent API with other primitive components (asChild, className, children)
- Uses
Primitive.spanfor proper rendering flexibilitysrc/core/primitives/RadioGroup/context/RadioGroupPrimitiveItemContext.tsx (1)
9-11: Clean context implementation for item state.The context is well-structured with:
- Clear interface definition for the boolean state
- Appropriate default value (false)
- Proper TypeScript typing
This provides a clean way to share selection state between the item and indicator components.
| aria-checked={value === selectedValue} | ||
| data-checked={value === selectedValue} | ||
| aria-required={required} | ||
| name={name} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary name attribute from button element
The name attribute on a button element doesn't serve any functional purpose for form submission. Based on the test file, it appears the name attribute should be on a hidden input element instead.
- name={name}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name={name} |
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx at line
35, remove the name attribute from the button element because it is unnecessary
and does not affect form submission. Instead, ensure that the name attribute is
correctly applied to the hidden input element as indicated by the test file to
maintain proper form behavior.
| <Primitive.button | ||
| disabled={groupDisabled || disabled} | ||
| onClick={() => setSelectedValue(value)} | ||
| onFocus={() => setSelectedValue(value)} |
There was a problem hiding this comment.
Remove onFocus handler to prevent unexpected selection changes
The onFocus handler changes the selected value when the button receives focus. This deviates from standard radio button behavior where selection should only change on user activation (click/space/enter), not on focus alone. This could cause accessibility issues and unexpected behavior during keyboard navigation.
- onFocus={() => setSelectedValue(value)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onFocus={() => setSelectedValue(value)} |
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx at line
30, remove the onFocus handler that sets the selected value on focus. This
handler causes the selection to change when the radio button gains focus, which
is not standard behavior. Ensure that selection only changes on explicit user
activation events like click, space, or enter, to maintain expected
accessibility and keyboard navigation behavior.
kotAPI
added
the
automerge
Screen.Recording.2025-07-18.133931.mp4
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests