fixes to radiogroup themed #1206
Conversation
WalkthroughThe changes refactor the RadioGroup component and its primitives by updating type definitions, simplifying context, and adjusting props and styling. The RadioGroup context now only includes a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RadioGroupRoot
participant RadioGroupContext
participant RadioGroupItem
User->>RadioGroupRoot: Render with variant, color, size, className
RadioGroupRoot->>RadioGroupContext: Provide { rootClass }
RadioGroupRoot->>RadioGroupItem: Render children with context
RadioGroupItem->>RadioGroupContext: Use rootClass for styling
User->>RadioGroupItem: Interact (e.g., select radio)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related issues
Possibly related PRs
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 (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)src/components/ui/RadioCards/fragments/RadioCardsItem.tsx (1)src/components/ui/RadioCards/context/RadioCardsContext.tsx (2)src/components/ui/RadioCards/fragments/RadioCardsRoot.tsx (4)🧬 Code Graph Analysis (1)src/components/ui/RadioCards/fragments/RadioCardsItem.tsx (1)
🪛 GitHub Check: lintsrc/components/ui/RadioCards/fragments/RadioCardsItem.tsx[warning] 2-2: ⏰ 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)
🔇 Additional comments (8)
✨ 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: 4
🔭 Outside diff range comments (1)
styles/themes/components/radio-group.scss (1)
12-46: Fix CSS selector structure for radio input styles.The radio input styles (width, height, positioning, pseudo-elements) appear to be directly applied to
.rad-ui-radio-group-iteminstead of the actual input element. This will cause incorrect styling as these properties should target the radio input, not the container.The CSS structure needs to be corrected to properly target the radio input:
.rad-ui-radio-group-item{ display: flex; align-items: center; gap: 12px; + + input[type="radio"] { width: 20px; height: 20px; position:relative; display:inline-flex; align-items:center; justify-content:center; flex-shrink:0; outline:none; margin:0; border:none; &::before{ content: ''; display: inline-block; width: 20px; height: 20px; border-radius: 50%; background-color: var(--rad-ui-color-accent-900); } &[data-checked="true"]{ &::after{ content: ''; } } &::after{ display: inline-block; position: absolute; transform: scale(0.4); width: 100%; height: 100%; border-radius: 50%; background-color: var(--rad-ui-color-accent-400); } + } }
🧹 Nitpick comments (3)
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx (1)
17-21: InternalisOpenstate never updates when theopenprop changes
useState(open)only captures the initial value. If the parent later togglesopen,isOpenstays stale, making the component behave uncontrolled even when the prop is supplied.Add a sync effect:
+ React.useEffect(() => { + setIsOpen(open); + }, [open]);or model the component as fully controlled/uncontrolled like other primitives in the library.
Does this component need to support controlled mode similar to
AccordionRoot(see learning 1)?styles/themes/components/radio-group.scss (1)
47-47: Remove trailing whitespace.- +src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
1-1: Remove unused import.The
RefObjectimport is not used in this file.-import { createContext, RefObject } from 'react'; +import { createContext } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx(1 hunks)src/components/ui/RadioGroup/stories/RadioGroup.stories.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx(1 hunks)src/core/primitives/Radio/tests/Radio.test.tsx(1 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(0 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(2 hunks)styles/jsTokens/colors.tokens.js(1 hunks)styles/themes/components/blockquote.scss(1 hunks)styles/themes/components/checkbox.scss(1 hunks)styles/themes/components/radio-group.scss(1 hunks)styles/themes/default.scss(1 hunks)
💤 Files with no reviewable changes (1)
- src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
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: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
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`.
styles/themes/components/checkbox.scss (2)
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#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
src/components/ui/RadioGroup/stories/RadioGroup.stories.tsx (1)
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/Dialog/fragments/DialogPrimitiveRoot.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: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
src/core/primitives/Radio/tests/Radio.test.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/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
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/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (1)
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/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (4)
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.
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: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
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.
🧬 Code Graph Analysis (2)
src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (1)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(7-9)
src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(7-9)
🪛 GitHub Actions: Build RAD UI
src/core/primitives/Radio/tests/Radio.test.tsx
[warning] 19-21: Primitive.input: asChild prop requires exactly one valid child element.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
[error] 33-33: TypeScript error TS2322: Property 'className' does not exist on type 'IntrinsicAttributes & RadioPrimitiveProps'.
🪛 GitHub Check: lint
src/components/ui/RadioGroup/context/RadioGroupContext.tsx
[warning] 1-1:
'RefObject' is defined but never used
🔇 Additional comments (13)
styles/themes/components/blockquote.scss (1)
5-7: Formatting-only change is fineBlank-line insertion improves readability and stays within the style guide.
styles/jsTokens/colors.tokens.js (1)
1-436: Numeric keys now rely on implicit string-coercion – confirm downstream consumersSwitching from quoted to bare numeric keys (e.g.
50:) is valid ES2015, but some TypeScript configs (noImplicitAny,esModuleInteropoff) or JSON loaders might choke when they previously expected string literals. Please run the build / typings generation pipeline to ensure no DTOs or token-parsing utilities break on this new syntax.styles/themes/components/checkbox.scss (1)
11-13: LGTM – whitespace tweak onlyNo functional impact; consistent with surrounding SCSS formatting.
styles/themes/default.scss (1)
25-29: Import rename aligns with component refactorThe theme now pulls
components/radio-group; the oldradiostylesheet is no longer referenced. Make surecomponents/radio-group.scssexists in the compiled bundle to avoid a missing-module error.src/core/primitives/Radio/tests/Radio.test.tsx (1)
6-48: Formatting changes look good.The indentation changes from 2 to 4 spaces improve consistency and readability.
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
3-9: Excellent refactoring to typed context interface.The refactoring from an untyped context to a properly typed interface with
RadioGroupContextTypeis a significant improvement. Simplifying the context to only includerootClassaligns well with the component simplification goals.src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (2)
32-32: Good addition of data-checked attribute.The
data-checkedattribute provides a clean way to style the radio button based on its checked state, which aligns well with the CSS changes in the theme file.
11-11: Good addition of className prop to component interface.Adding optional
classNameprop enhances styling flexibility for the radio group items.src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (3)
11-15: Good simplification of component props.Removing the unused
defaultCheckedandonChangeprops simplifies the component interface and aligns with the context refactoring.
20-20: Context provider value consistent with typed interface.The context provider now only passes
rootClass, which matches the newRadioGroupContextTypeinterface perfectly.
17-23: State management is implemented in the primitive root—no action needed.The
useControllableStatehook in
•src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
handlesvalue,defaultValue, andonValueChange, and provides the selection context for the radio group.src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (2)
6-6: Excellent type safety improvement!The explicit props interface is much better than the previous spread approach. Making
valuerequired andclassNameoptional with proper typing improves developer experience and prevents runtime errors.
8-8: Consistent with the explicit props interface.Explicitly passing the
valueprop ensures it's always provided to the primitive component, which aligns perfectly with making it required in the props type. The className handling withclsxand context integration looks correct.
| const COMPONENT_NAME = 'DialogPrimitive'; | ||
|
|
||
| const DialogPrimitiveRoot = ({ children, open=false, onOpenChange = () => {}, onClickOutside = () => {}, className, ...props } : DialogPrimitiveRootProps) => { | ||
| const DialogPrimitiveRoot = ({ children, open = false, onOpenChange = () => {}, onClickOutside = () => {}, className, ...props } : DialogPrimitiveRootProps) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Spread ...props escapes the declared interface – TypeScript will complain
DialogPrimitiveRootProps does not include an index signature or extend any HTML-prop type, yet the function spreads ...props onto the <div>. This produces an “Object literal may only specify known properties” error when consumers pass e.g. id, style, data-*, etc.
-export type DialogPrimitiveRootProps = {
- children: React.ReactNode;
- open?: boolean;
- onOpenChange?: (open: boolean) => void;
- onClickOutside?: () => void;
- className?:string
-}
+export type DialogPrimitiveRootProps =
+ React.HTMLAttributes<HTMLDivElement> & {
+ children: React.ReactNode;
+ open?: boolean;
+ onOpenChange?: (open: boolean) => void;
+ onClickOutside?: () => void;
+ className?: string;
+ };Afterwards keep ...rest (rename for clarity) and spread it on the wrapper <div>.
📝 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.
| const DialogPrimitiveRoot = ({ children, open = false, onOpenChange = () => {}, onClickOutside = () => {}, className, ...props } : DialogPrimitiveRootProps) => { | |
| // Before: props weren’t allowed on <div> because DialogPrimitiveRootProps didn’t extend HTML props | |
| -export type DialogPrimitiveRootProps = { | |
| - children: React.ReactNode; | |
| - open?: boolean; | |
| - onOpenChange?: (open: boolean) => void; | |
| - onClickOutside?: () => void; | |
| - className?:string | |
| -} | |
| +export type DialogPrimitiveRootProps = | |
| + React.HTMLAttributes<HTMLDivElement> & { | |
| + children: React.ReactNode; | |
| + open?: boolean; | |
| + onOpenChange?: (open: boolean) => void; | |
| + onClickOutside?: () => void; | |
| + className?: string; | |
| + }; | |
| // Before: was using `...props` | |
| -const DialogPrimitiveRoot = ({ children, open = false, onOpenChange = () => {}, onClickOutside = () => {}, className, ...props } : DialogPrimitiveRootProps) => { | |
| +const DialogPrimitiveRoot = ({ | |
| + children, | |
| + open = false, | |
| + onOpenChange = () => {}, | |
| + onClickOutside = () => {}, | |
| + className, | |
| + ...rest | |
| +}: DialogPrimitiveRootProps) => { | |
| return ( | |
| - <div className={className} {...props}> | |
| + <div className={className} {...rest}> | |
| {children} | |
| </div> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx at line 16, the
component spreads ...props onto the div but DialogPrimitiveRootProps does not
allow arbitrary HTML attributes, causing TypeScript errors. To fix this, update
the DialogPrimitiveRootProps interface to extend
React.HTMLAttributes<HTMLDivElement> so it accepts standard div props, rename
...props to ...rest for clarity, and spread ...rest on the div element.
| <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > | ||
| {options.map((option) => ( | ||
| <RadioGroup.Item key={option.id} value={option.value}> | ||
| <RadioGroup.Item key={option.id} value={option.value} > | ||
| {option.label} |
There was a problem hiding this comment.
Story still uses removed props – will not compile
defaultChecked and onChange were removed from RadioGroup.Root in this PR, yet the story keeps them. Update to the new controlled API (value / onValueChange) or whatever the replacement is in RadioGroupRoot.
- const [language, setLanguage] = useState('css');
- const handleChange = (e) => {
- setLanguage(e.target.value);
- };
+ const [language, setLanguage] = useState('css');
+ const handleChange = (newValue: string) => {
+ setLanguage(newValue);
+ };
...
- <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} >
+ <RadioGroup.Root value={language} items={options} onValueChange={handleChange}>Failing to adjust this will break Storybook and CI.
📝 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.
| <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > | |
| {options.map((option) => ( | |
| <RadioGroup.Item key={option.id} value={option.value}> | |
| <RadioGroup.Item key={option.id} value={option.value} > | |
| {option.label} | |
| const [language, setLanguage] = useState('css'); | |
| const handleChange = (newValue: string) => { | |
| setLanguage(newValue); | |
| }; | |
| <RadioGroup.Root value={language} items={options} onValueChange={handleChange}> | |
| {options.map((option) => ( | |
| <RadioGroup.Item key={option.id} value={option.value}> | |
| {option.label} | |
| </RadioGroup.Item> | |
| ))} | |
| </RadioGroup.Root> |
🤖 Prompt for AI Agents
In src/components/ui/RadioGroup/stories/RadioGroup.stories.tsx around lines 18
to 21, the story uses removed props defaultChecked and onChange on
RadioGroup.Root, which will cause compilation errors. Replace defaultChecked
with the controlled prop value set to language, and replace onChange with
onValueChange set to handleChange, updating the story to use the new controlled
API for RadioGroup.Root.
| it('supports asChild prop (renders without error)', () => { | ||
| render(<RadioPrimitive {...baseProps} asChild />); | ||
| const radio = screen.getByRole('radio'); | ||
| expect(radio).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Fix asChild test to provide required child element.
The pipeline failure indicates that the asChild prop requires exactly one valid child element. The current test doesn't provide any child, which violates the asChild contract.
- it('supports asChild prop (renders without error)', () => {
- render(<RadioPrimitive {...baseProps} asChild />);
- const radio = screen.getByRole('radio');
- expect(radio).toBeInTheDocument();
- });
+ it('supports asChild prop (renders without error)', () => {
+ render(
+ <RadioPrimitive {...baseProps} asChild>
+ <input type="radio" />
+ </RadioPrimitive>
+ );
+ const radio = screen.getByRole('radio');
+ expect(radio).toBeInTheDocument();
+ });📝 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.
| it('supports asChild prop (renders without error)', () => { | |
| render(<RadioPrimitive {...baseProps} asChild />); | |
| const radio = screen.getByRole('radio'); | |
| expect(radio).toBeInTheDocument(); | |
| }); | |
| it('supports asChild prop (renders without error)', () => { | |
| render( | |
| <RadioPrimitive {...baseProps} asChild> | |
| <input type="radio" /> | |
| </RadioPrimitive> | |
| ); | |
| const radio = screen.getByRole('radio'); | |
| expect(radio).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In src/core/primitives/Radio/tests/Radio.test.tsx around lines 43 to 47, the
test using the asChild prop is missing a required child element. Update the test
to wrap the RadioPrimitive component with a single valid child element, such as
a span or div, to satisfy the asChild prop contract and prevent pipeline
failures.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx(1 hunks)src/components/ui/RadioGroup/stories/RadioGroup.stories.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(3 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(3 hunks)styles/themes/components/radio-group.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- styles/themes/components/radio-group.scss
- src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx
- src/components/ui/RadioGroup/stories/RadioGroup.stories.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
- src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx
- src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
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/fragments/RadioGroupPrimitiveRoot.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: 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.
🔇 Additional comments (3)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (3)
7-18: LGTM: Type export improves reusabilityExporting the
RadioGroupPrimitiveRootPropstype is a good change that allows for better type reusability across the codebase, aligning with the broader refactor mentioned in the AI summary.
35-35: Good: Explicit prop passing improves clarityThe explicit passing of
dir,orientation, andloopprops toRovingFocusGroup.Rootimproves code clarity and ensures these props are properly forwarded to the underlying focus management system.
38-38: LGTM: className prop aligns with refactoring goalsThe addition of the
classNameprop to thePrimitive.divelement aligns well with the broader refactor to streamline class name handling, replacing the previouscustomRootClassapproach mentioned in the AI summary.
| }>; | ||
|
|
||
| const RadioGroupPrimitiveRoot = ({ value, defaultValue = '', onValueChange, children, disabled: groupDisabled = false, required = false, name = '', orientation = 'horizontal', loop = false, dir = 'ltr', ...props }: RadioGroupPrimitiveRootProps) => { | ||
| const RadioGroupPrimitiveRoot = ({ value, defaultValue = '', onValueChange, children, disabled: groupDisabled = false, required = false, name = '', orientation = 'both', loop = true, dir = 'ltr', className = '', ...props }: RadioGroupPrimitiveRootProps) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the breaking changes in default prop values
The default values for orientation and loop have been changed:
orientation:'horizontal'→'both'loop:false→true
These changes could potentially break existing implementations that rely on the previous defaults.
Please verify that these default value changes are intentional and documented, and ensure backward compatibility is maintained:
🏁 Script executed:
#!/bin/bash
# Description: Search for RadioGroup usage to assess impact of default prop changes
# Expected: Find all RadioGroup usages to verify if they explicitly set these props
# Search for RadioGroup component usage patterns
rg -A 5 -B 2 "RadioGroup" --type tsx --type ts
# Search for any existing tests that might rely on the old defaults
rg -A 10 -B 2 "(orientation|loop)" --type tsx --type ts | grep -i test
# Look for any documentation that mentions these defaults
fd -e md -e mdx | xargs rg -l "RadioGroup" | head -5 | xargs rg -A 5 -B 5 "(orientation|loop)"Length of output: 95216
Document and Communicate Updated RadioGroupPrimitive Defaults
Verified that the defaults for orientation ('horizontal'→'both') and loop (false→true) in RadioGroupPrimitiveRoot are intentional and no internal code or tests depend on the old values. To ensure users aren’t caught off-guard, please:
- Update the JSDoc/API comments in
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
• Reflect defaults:orientation = 'both',loop = true - Add or update the public API documentation (e.g. under
docs/app/docs/components/primitives/RadioGroupor your docs directory) to call out the new defaults - Record this breaking change in the CHANGELOG or release notes
- (Optional) Add a basic interaction test for the default focus behavior (no props passed)
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx at line
20, update the JSDoc/API comments to reflect the new default values orientation
= 'both' and loop = true. Additionally, update the public API documentation in
the docs directory to clearly state these new defaults. Record this change as a
breaking change in the CHANGELOG or release notes. Optionally, add a basic
interaction test verifying the default focus behavior when no props are passed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/core/primitives/Radio/tests/Radio.test.tsx (2)
37-47: Remove duplicate test case.This test is identical to the one at lines 26-36. Having duplicate tests reduces maintainability and can cause confusion.
- it('applies checked, required, and disabled props', () => { - render( - <RadioPrimitive {...baseProps} checked required disabled /> - ); - const radio = screen.getByRole('radio'); - expect(radio).toBeChecked(); - expect(radio).toBeRequired(); - expect(radio).toBeDisabled(); - expect(radio).toHaveAttribute('aria-disabled', 'true'); - expect(radio).toHaveAttribute('aria-required', 'true'); - });
58-66: Remove duplicate test case.This test is identical to the one at lines 49-57. Having duplicate tests reduces maintainability and can cause confusion.
- it('calls onChange when clicked', () => { - const handleChange = jest.fn(); - render( - <RadioPrimitive {...baseProps} onChange={handleChange} /> - ); - const radio = screen.getByRole('radio'); - fireEvent.click(radio); - expect(handleChange).toHaveBeenCalled(); - });
♻️ Duplicate comments (1)
src/core/primitives/Radio/tests/Radio.test.tsx (1)
73-77: Remove duplicate test case and fix asChild implementation.This test is duplicated and still has the same issue flagged in previous reviews - the asChild prop requires a child element.
- it('supports asChild prop (renders without error)', () => { - render(<RadioPrimitive {...baseProps} asChild />); - const radio = screen.getByRole('radio'); - expect(radio).toBeInTheDocument(); - });The remaining asChild test (lines 68-72) should also be fixed to provide the required child element as indicated in the previous review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/primitives/Radio/tests/Radio.test.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/core/primitives/Radio/tests/Radio.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`.
🪛 Biome (1.9.4)
src/core/primitives/Radio/tests/Radio.test.tsx
[error] 11-11: Shouldn't redeclare 'baseProps'. Consider to delete it or rename it.
'baseProps' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: Build RAD UI
src/core/primitives/Radio/tests/Radio.test.tsx
[error] 6-11: Test suite failed to run: Cannot redeclare block-scoped variable 'baseProps'.
🪛 GitHub Actions: Lint
src/core/primitives/Radio/tests/Radio.test.tsx
[error] 11-11: Parsing error: Identifier 'baseProps' has already been declared.
|
closing as stale and marked for revert |
closes #1194
also closes #1179 and #1178
currently WIP
Screen.Recording.2025-07-10.231915.mp4
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Style