refactor RadioGroup to forward refs #1471
Conversation
WalkthroughRefactors RadioGroup and RadioGroupPrimitive to forwardRef-based components with explicit div-based props and exported Element ref types. Consolidates public API to a single component exposing static subcomponents. Updates tests to assert ref forwarding, including for Label. Adds displayName assignments and public type aliases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant RG as RadioGroup (forwardRef)
participant Root as RadioGroup.Root
participant Item as RadioGroup.Item
participant Ind as RadioGroup.Indicator
participant Lab as RadioGroup.Label
participant DOM as DOM Elements
App->>RG: Render <RadioGroup ref={groupRef}>...</RadioGroup>
RG-->>App: forwards ref (div)
App->>Root: Render
App->>Item: Render (n)
App->>Ind: Render
App->>Lab: Render with ref={labelRef}
Lab-->>App: forwards ref (label)
note over RG,Lab: New: explicit forwardRef typing and static subcomponents
Root->>DOM: mount div
Item->>DOM: mount item elements
Ind->>DOM: mount indicator
Lab->>DOM: mount label
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (6)
src/components/ui/RadioGroup/tests/RadioGroup.test.tsx (1)
102-123: Ref forwarding test reads well; consider minor ergonomics tweaksLooks good and validates the intended HTML element types. Optional:
- Prefer user-event over fireEvent for realistic interactions elsewhere in the suite.
- For extra robustness across environments, you can add a fallback assertion like expect(labelRef.current?.tagName).toBe('LABEL') in addition to instanceof checks.
src/components/ui/RadioGroup/RadioGroup.tsx (2)
9-11: Redundant children in propschildren is already included via React.ComponentPropsWithoutRef<'div'>. You can simplify.
-export type RadioGroupProps = React.ComponentPropsWithoutRef<'div'> & { - children?: React.ReactNode; -}; +export type RadioGroupProps = React.ComponentPropsWithoutRef<'div'>;
20-23: Gate console.warn to dev-onlyAvoid noisy logs in production while keeping DX in dev.
-const RadioGroup = React.forwardRef<RadioGroupElement, RadioGroupProps>((_props, _ref) => { - console.warn('Direct usage of RadioGroup is not supported. Please use RadioGroup.Root and RadioGroup.Item instead.'); - return null; -}) as RadioGroupComponent; +const RadioGroup = React.forwardRef<RadioGroupElement, RadioGroupProps>((_props, _ref) => { + if (process.env.NODE_ENV !== 'production') { + console.warn('Direct usage of RadioGroup is not supported. Please use RadioGroup.Root and RadioGroup.Item instead.'); + } + return null; +}) as RadioGroupComponent;src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (3)
7-12: Redundant children in propsSame note as UI layer; children is already present on div props.
-export type RadioGroupProps = React.ComponentPropsWithoutRef<'div'> & { - children?: React.ReactNode; -}; +export type RadioGroupProps = React.ComponentPropsWithoutRef<'div'>;
19-23: Fix warning copy; mention the correct component and gate to devThe message references “RadioGroup” instead of “RadioGroupPrimitive”, which can confuse users. Also, avoid logging in production.
-const RadioGroupPrimitive = React.forwardRef<RadioGroupPrimitiveElement, RadioGroupProps>((_props, _ref) => { - console.warn('Direct usage of RadioGroup is not supported. Please use RadioGroup.Root, RadioGroup.Item, etc. instead.'); - return null; -}) as RadioGroupPrimitiveComponent; +const RadioGroupPrimitive = React.forwardRef<RadioGroupPrimitiveElement, RadioGroupProps>((_props, _ref) => { + if (process.env.NODE_ENV !== 'production') { + console.warn( + 'Direct usage of RadioGroupPrimitive is not supported. Please use RadioGroupPrimitive.Root, RadioGroupPrimitive.Item, etc. instead.' + ); + } + return null; +}) as RadioGroupPrimitiveComponent;
13-17: Composite type with statics is fine; consider centralizing warn copyOptional: export a shared WARN_MESSAGE constant to keep UI and primitive messages in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/components/ui/RadioGroup/RadioGroup.tsx(1 hunks)src/components/ui/RadioGroup/tests/RadioGroup.test.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/ui/RadioGroup/RadioGroup.tsx (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
RadioGroupProps(9-11)
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
src/components/ui/RadioGroup/RadioGroup.tsx (1)
RadioGroupProps(9-11)
⏰ 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 (5)
src/components/ui/RadioGroup/tests/RadioGroup.test.tsx (1)
39-39: No-op change; nothing to review here.src/components/ui/RadioGroup/RadioGroup.tsx (2)
7-18: Type surface for element and composite component looks solidExported RadioGroupElement and the composite type with static fragments are correct and IDE-friendly.
27-30: Static fragment exposure is clearAssignments for Root/Item/Indicator/Label are straightforward and match the public API.
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (2)
26-30: Nice DX: namespaced prop types for subcomponentsThe RadioGroupPrimitiveProps namespace is a helpful discoverability touch.
7-12: No conflicting barrel exports for RadioGroupProps
I didn’t find any index file re-exporting both UI and PrimitiveRadioGroupProps, so there’s no import-name collision risk.
Summary
Testing
npm testnpm run build:rollupSummary by CodeRabbit
New Features
Refactor
Tests