Refactor ToggleGroup to use RovingFocusGroup implementation#938
Conversation
WalkthroughThe pull request standardizes formatting across several UI components and tests, such as using single quotes for strings and consistent spacing in destructuring assignments. It refines the Toggle component by making its Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toggle as Toggle Component
participant Hook as useControllableState
participant Parent as Parent Component
User->>Toggle: Click or toggle action
Toggle->>Hook: Invoke setValue(newValue)
alt Controlled Mode
Hook-->>Toggle: Return controlled value
Toggle->>Parent: Trigger onChange(newValue)
else Uncontrolled Mode
Hook-->>Toggle: Update internal state
Toggle->>Parent: Trigger onChange(newValue) (if provided)
end
sequenceDiagram
participant User
participant TG as ToggleGroup Component
participant RFG as RovingFocusGroup
User->>TG: Focus toggle item / Use arrow key
TG->>RFG: Delegate focus management
RFG-->>TG: Return updated focused item
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🔭 Outside diff range comments (1)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
17-21:⚠️ Potential issueFix missing context properties causing build failure.
The
sendValuesobject is missing the requirednextItemandpreviousItemproperties that are defined in theToggleContexttype, causing a TypeScript error.const sendValues = { activeToggles, setActiveToggles, type + nextItem: () => {}, + previousItem: () => {} };Consider whether these functions are still needed in the context, as you're now using
RovingFocusGroupfor focus management. If they're no longer needed, update the context type definition instead.
🧹 Nitpick comments (3)
src/components/ui/Toggle/Toggle.tsx (1)
38-39: Helpful transitional commentsThese comments explain why validation was removed, which is helpful for understanding the changes. Consider whether these comments will remain valuable long-term or if they should be removed after the PR is merged.
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
54-61: Improved focus management with RovingFocusGroupWrapping
TogglePrimitivewithRovingFocusGroup.Itemis a good refactoring that removes the need for manual focus and keyboard event handling. This likely improves accessibility and reduces code complexity.Consider improving the JSX formatting for consistency by adding proper indentation:
- return <RovingFocusGroup.Item> - <TogglePrimitive - onClick={handleToggleSelect} - {...ariaProps} - {...dataProps} - {...props} - >{children}</TogglePrimitive> - </RovingFocusGroup.Item>; + return ( + <RovingFocusGroup.Item> + <TogglePrimitive + onClick={handleToggleSelect} + {...ariaProps} + {...dataProps} + {...props} + > + {children} + </TogglePrimitive> + </RovingFocusGroup.Item> + );src/components/ui/Toggle/stories/Toggle.stories.tsx (1)
238-242: Consider adding aria-label for icon-only toggles.For better accessibility, icon-only toggles should include an aria-label to describe their purpose to screen reader users.
<Toggle defaultPressed={false} onChange={() => {}} + aria-label="Toggle move function" > <MoveIcon /> </Toggle>Also applies to: 248-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/ui/Button/Button.tsx(2 hunks)src/components/ui/Code/Code.tsx(1 hunks)src/components/ui/Code/stories/Code.stories.tsx(3 hunks)src/components/ui/Toggle/Toggle.tsx(2 hunks)src/components/ui/Toggle/stories/Toggle.stories.tsx(1 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx(3 hunks)src/core/hooks/createDataAttribute/createDataAttribute.test.tsx(1 hunks)src/core/hooks/createDataAttribute/index.ts(1 hunks)src/core/hooks/useControllableState/index.tsx(1 hunks)src/core/hooks/useControllableState/useControllableState.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/components/ui/Toggle/Toggle.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2025-03-13T06:35:04.614Z
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 Definitions (6)
src/core/hooks/createDataAttribute/createDataAttribute.test.tsx (1)
src/core/hooks/createDataAttribute/index.ts (2) (2)
useCreateDataAttribute(10-25)useComposeAttributes(33-40)
src/core/hooks/useControllableState/useControllableState.test.tsx (1)
src/core/hooks/useControllableState/index.tsx (1) (1)
useControllableState(15-78)
src/components/ui/Button/Button.tsx (1)
src/core/hooks/createDataAttribute/index.ts (1) (1)
useCreateDataAttribute(10-25)
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1) (1)
ToggleContext(11-17)
src/components/ui/Toggle/Toggle.tsx (1)
src/core/hooks/useControllableState/index.tsx (1) (1)
useControllableState(15-78)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1) (1)
ToggleContext(11-17)
🪛 GitHub Check: lint
src/components/ui/Toggle/stories/Toggle.stories.tsx
[warning] 201-201:
A form label must be associated with a control
[warning] 192-192:
A form label must be associated with a control
[warning] 183-183:
A form label must be associated with a control
🪛 GitHub Actions: Build RAD UI
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx
[error] 30-33: TS2739: Type '{ activeToggles: any; setActiveToggles: Dispatch; type: any; }' is missing the following properties from type 'ToggleContextType': nextItem, previousItem
🔇 Additional comments (39)
src/components/ui/Code/Code.tsx (1)
16-16: Consistent formatting looks good!The spacing around default parameter assignments has been standardized, which enhances code readability.
src/components/ui/Code/stories/Code.stories.tsx (3)
6-7: Formatting improvement for better readability!Adding spaces after commas in arrays improves readability and follows standard formatting practices.
56-58: Consistent indentation improves code readability!The proper indentation of JSX content helps maintain a clear hierarchical structure.
76-80: Well-structured JSX formatting!The consistent indentation in the mapping function's return statement improves readability.
src/core/hooks/createDataAttribute/index.ts (5)
1-1: Standardized quote usage!Using single quotes for string literals is more consistent with the project's style.
5-5: Improved JSDoc formatting!Adding a line break after the first sentence in JSDoc comments improves readability.
10-13: Better function parameter indentation!The improved indentation of function parameters enhances code readability.
14-24: Consistent function body indentation!The function body is now properly indented, making the code structure clearer.
33-40: Consistent indentation for useComposeAttributes!The improved indentation for parameters and function body enhances readability.
src/core/hooks/createDataAttribute/createDataAttribute.test.tsx (2)
1-2: Standardized import formatting!Using single quotes for imports maintains consistency with the project's style.
7-53: Consistent string quotation style!Converting all string literals to use single quotes improves code style consistency throughout the test file.
src/core/hooks/useControllableState/useControllableState.test.tsx (1)
1-74: Great comprehensive test coverage for the useControllableState hook!The tests thoroughly cover both uncontrolled and controlled modes of the hook, including:
- Default value initialization
- Value updates
- Callback handling
- Functional updates
This provides robust validation for a hook that will be critical for managing state in the refactored Toggle component.
src/components/ui/Toggle/Toggle.tsx (4)
4-4: Implements controlled and uncontrolled state patterns with new hookThe addition of
useControllableStatehook enhances the component by enabling proper controlled and uncontrolled behavior patterns.
12-12: LGTM, makes Toggle more flexibleMaking the
pressedprop optional is a good enhancement that allows the component to operate in both controlled and uncontrolled modes. The comment clearly explains the rationale.
32-36: Clean implementation of state managementThe implementation using
useControllableStateis clean and follows best practices for React components that need to support both controlled and uncontrolled modes. This approach appropriately manages the internal state based on prop presence.
53-53: Simplified event handlingDirectly passing
setIsPressedtoonPressedChangesimplifies the code by eliminating the need for an intermediate handler function. This is a good refactoring that makes the code more maintainable.src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (2)
5-5: Good upgrade to use RovingFocusGroupImporting and utilizing
RovingFocusGroupis a solid improvement that delegates focus management to a specialized utility rather than implementing it manually.
14-14: Simplified context usageThe context usage has been simplified to only extract what's needed after removing focus management code. This follows good practices by minimizing dependencies.
src/components/ui/Button/Button.tsx (2)
6-6: Improved formatting consistencyRemoving extra spaces in the import statement improves code formatting consistency.
22-23: Standardized string quotesChanging from double quotes to single quotes standardizes the string literals format in the codebase, improving consistency.
src/components/ui/Toggle/tests/Toggle.test.js (7)
7-7: Simplified component initializationRemoving the
pressedprop appropriately tests the component in its uncontrolled state, which aligns with the component changes.
12-12: Updated component initialization for uncontrolled modeProperly updated test cases to use the component in uncontrolled mode, aligning with the main component changes.
Also applies to: 17-17
21-41: Comprehensive test for controlled modeThis test case thoroughly verifies the behavior of the controlled mode:
- Confirms initial state matches the prop
- Verifies callback is called with the expected value when clicked
- Checks that the internal state doesn't change until props are updated
- Confirms the state updates correctly after props change
This is a good pattern for testing controlled components.
44-68: Well-structured test for uncontrolled modeThe new test case for uncontrolled mode appropriately verifies:
- Initial state based on defaultPressed
- Internal state updates when the toggle is clicked
- Callback is called with the correct values
- Toggle behavior works in both directions
This thoroughly covers the uncontrolled behavior of the component.
70-73: Simplified disabled state testThe test for disabled state has been appropriately simplified while still verifying the essential behavior.
75-80: Added test for defaultPressedGood addition of a specific test for the defaultPressed prop in uncontrolled mode, ensuring this essential functionality is verified.
83-83: Updated component initialization in color testProperly updated the color test to use the component in uncontrolled mode.
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (3)
1-5: Good refactoring to use RovingFocusGroup for standardized accessibility.The integration of
RovingFocusGroupfrom core utils is a good decision as it provides standardized keyboard navigation and focus management, improving the component's accessibility.
9-9: New direction prop enhances component flexibility.Adding the
directionprop with default 'horizontal' improves the component's versatility by allowing both horizontal and vertical toggle group orientations.
29-36: Well-structured integration of RovingFocusGroup.The implementation correctly wraps the toggle group with
RovingFocusGroupcomponents and passes appropriate props. Good job movingdata-attributesto the Root component and maintaining className handling on the Group component.🧰 Tools
🪛 GitHub Actions: Build RAD UI
[error] 30-33: TS2739: Type '{ activeToggles: any; setActiveToggles: Dispatch; type: any; }' is missing the following properties from type 'ToggleContextType': nextItem, previousItem
src/core/hooks/useControllableState/index.tsx (5)
3-14: Well-documented hook with clear purpose and parameters.The JSDoc comments are thorough and clearly explain the hook's purpose, parameters, and return values.
15-24: Type-safe implementation with proper generics usage.The hook correctly uses TypeScript generics to ensure type safety while remaining flexible for any value type. The implementation properly initializes state and tracks whether the component is operating in controlled or uncontrolled mode.
26-38: Helpful development-only warnings for mode changes.The warning logic for detecting switches between controlled and uncontrolled modes will help developers identify potential issues during development.
40-54: Good practice to warn about controlled value changes without handlers.This warning helps prevent the common React anti-pattern of changing a controlled value without providing an onChange handler.
56-75: Robust setValue implementation with proper memoization.The setValue function correctly handles both direct values and updater functions, while properly managing state based on the component mode. The useCallback memoization prevents unnecessary re-renders.
src/components/ui/Toggle/stories/Toggle.stories.tsx (4)
8-25: Excellent component documentation with clear usage patterns.The storybook parameters include comprehensive documentation that clearly explains the controlled vs. uncontrolled usage modes of the Toggle component.
32-70: Great controlled mode example with interactive state control.This story effectively demonstrates the controlled usage pattern with React state and includes interactive buttons to manipulate the state from outside the component.
72-98: Clear uncontrolled example showing internal state management.The implementation clearly demonstrates how to use the Toggle component in uncontrolled mode with the defaultPressed prop.
318-323: Good demonstration of disabled toggle states.The stories effectively demonstrate disabled toggles in both on and off states, which is helpful for understanding the component's visual states.
Also applies to: 343-348
| export const FormIntegration = () => { | ||
| const [formData, setFormData] = React.useState({ | ||
| notifications: false, | ||
| darkMode: true, | ||
| autoSave: false | ||
| }); | ||
|
|
||
| const handleToggle = (field: keyof typeof formData, value: boolean) => { | ||
| setFormData(prev => ({ | ||
| ...prev, | ||
| [field]: value | ||
| })); | ||
| }; | ||
|
|
||
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| alert(`Form submitted with: ${JSON.stringify(formData, null, 2)}`); | ||
| }; | ||
|
|
||
| return ( | ||
| <SandboxEditor className="space-y-4 pt-4"> | ||
| <div className="mb-2 text-sm font-medium">Form Integration (Controlled)</div> | ||
| <div className="text-xs text-gray-600 mb-4"> | ||
| Example showing toggles integrated into a form with controlled state. | ||
| </div> | ||
|
|
||
| <form onSubmit={handleSubmit} className="space-y-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <label className="text-sm">Enable Notifications</label> | ||
| <Toggle | ||
| pressed={formData.notifications} | ||
| onChange={(value) => handleToggle('notifications', value)} | ||
| color="blue" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex items-center justify-between"> | ||
| <label className="text-sm">Dark Mode</label> | ||
| <Toggle | ||
| pressed={formData.darkMode} | ||
| onChange={(value) => handleToggle('darkMode', value)} | ||
| color="purple" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex items-center justify-between"> | ||
| <label className="text-sm">Auto-Save</label> | ||
| <Toggle | ||
| pressed={formData.autoSave} | ||
| onChange={(value) => handleToggle('autoSave', value)} | ||
| color="green" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="pt-4"> | ||
| <button | ||
| type="submit" | ||
| className="px-4 py-2 bg-blue-500 text-white rounded hover:bg-blue-600" | ||
| > | ||
| Save Preferences | ||
| </button> | ||
| </div> | ||
| </form> | ||
|
|
||
| <div className="text-sm mt-4 p-2 bg-gray-100 rounded"> | ||
| <pre className="text-xs">{JSON.stringify(formData, null, 2)}</pre> | ||
| </div> | ||
| </SandboxEditor> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Form labels need proper associations with controls.
The form implementation is good, but there are accessibility issues with the labels not being properly associated with their controls.
Update the form labels to use proper associations with their corresponding Toggle components:
- <label className="text-sm">Enable Notifications</label>
+ <label className="text-sm" id="notifications-label">Enable Notifications</label>
<Toggle
pressed={formData.notifications}
onChange={(value) => handleToggle('notifications', value)}
color="blue"
+ aria-labelledby="notifications-label"
/>Apply similar changes to the other form labels and Toggle components for proper accessibility.
📝 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.
| export const FormIntegration = () => { | |
| const [formData, setFormData] = React.useState({ | |
| notifications: false, | |
| darkMode: true, | |
| autoSave: false | |
| }); | |
| const handleToggle = (field: keyof typeof formData, value: boolean) => { | |
| setFormData(prev => ({ | |
| ...prev, | |
| [field]: value | |
| })); | |
| }; | |
| const handleSubmit = (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| alert(`Form submitted with: ${JSON.stringify(formData, null, 2)}`); | |
| }; | |
| return ( | |
| <SandboxEditor className="space-y-4 pt-4"> | |
| <div className="mb-2 text-sm font-medium">Form Integration (Controlled)</div> | |
| <div className="text-xs text-gray-600 mb-4"> | |
| Example showing toggles integrated into a form with controlled state. | |
| </div> | |
| <form onSubmit={handleSubmit} className="space-y-4"> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm">Enable Notifications</label> | |
| <Toggle | |
| pressed={formData.notifications} | |
| onChange={(value) => handleToggle('notifications', value)} | |
| color="blue" | |
| /> | |
| </div> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm">Dark Mode</label> | |
| <Toggle | |
| pressed={formData.darkMode} | |
| onChange={(value) => handleToggle('darkMode', value)} | |
| color="purple" | |
| /> | |
| </div> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm">Auto-Save</label> | |
| <Toggle | |
| pressed={formData.autoSave} | |
| onChange={(value) => handleToggle('autoSave', value)} | |
| color="green" | |
| /> | |
| </div> | |
| <div className="pt-4"> | |
| <button | |
| type="submit" | |
| className="px-4 py-2 bg-blue-500 text-white rounded hover:bg-blue-600" | |
| > | |
| Save Preferences | |
| </button> | |
| </div> | |
| </form> | |
| <div className="text-sm mt-4 p-2 bg-gray-100 rounded"> | |
| <pre className="text-xs">{JSON.stringify(formData, null, 2)}</pre> | |
| </div> | |
| </SandboxEditor> | |
| ); | |
| }; | |
| export const FormIntegration = () => { | |
| const [formData, setFormData] = React.useState({ | |
| notifications: false, | |
| darkMode: true, | |
| autoSave: false | |
| }); | |
| const handleToggle = (field: keyof typeof formData, value: boolean) => { | |
| setFormData(prev => ({ | |
| ...prev, | |
| [field]: value | |
| })); | |
| }; | |
| const handleSubmit = (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| alert(`Form submitted with: ${JSON.stringify(formData, null, 2)}`); | |
| }; | |
| return ( | |
| <SandboxEditor className="space-y-4 pt-4"> | |
| <div className="mb-2 text-sm font-medium">Form Integration (Controlled)</div> | |
| <div className="text-xs text-gray-600 mb-4"> | |
| Example showing toggles integrated into a form with controlled state. | |
| </div> | |
| <form onSubmit={handleSubmit} className="space-y-4"> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm" id="notifications-label">Enable Notifications</label> | |
| <Toggle | |
| pressed={formData.notifications} | |
| onChange={(value) => handleToggle('notifications', value)} | |
| color="blue" | |
| aria-labelledby="notifications-label" | |
| /> | |
| </div> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm" id="darkMode-label">Dark Mode</label> | |
| <Toggle | |
| pressed={formData.darkMode} | |
| onChange={(value) => handleToggle('darkMode', value)} | |
| color="purple" | |
| aria-labelledby="darkMode-label" | |
| /> | |
| </div> | |
| <div className="flex items-center justify-between"> | |
| <label className="text-sm" id="autoSave-label">Auto-Save</label> | |
| <Toggle | |
| pressed={formData.autoSave} | |
| onChange={(value) => handleToggle('autoSave', value)} | |
| color="green" | |
| aria-labelledby="autoSave-label" | |
| /> | |
| </div> | |
| <div className="pt-4"> | |
| <button | |
| type="submit" | |
| className="px-4 py-2 bg-blue-500 text-white rounded hover:bg-blue-600" | |
| > | |
| Save Preferences | |
| </button> | |
| </div> | |
| </form> | |
| <div className="text-sm mt-4 p-2 bg-gray-100 rounded"> | |
| <pre className="text-xs">{JSON.stringify(formData, null, 2)}</pre> | |
| </div> | |
| </SandboxEditor> | |
| ); | |
| }; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 201-201:
A form label must be associated with a control
[warning] 192-192:
A form label must be associated with a control
[warning] 183-183:
A form label must be associated with a control
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/ToggleGroup/tests/ToggleGroup.test.js (1)
27-27: Consider adding onChange callback verifications.These lines clearly test multiple selection states by clicking items and verifying the pressed attributes. To bolster coverage, you may want to confirm that any onChange callback (if present) is triggered with the correct toggles.
Also applies to: 29-31, 33-35, 37-38, 40-41
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
9-9: New ‘direction’ prop expands usage flexibility.The introduction of a
directionprop enhances orientation control. Consider validating its possible values (e.g., restricting to 'horizontal' or 'vertical') to prevent unexpected usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1) (1)
ToggleContext(9-13)
🔇 Additional comments (8)
src/components/ui/ToggleGroup/tests/ToggleGroup.test.js (5)
13-13: Great usage ofgetAllByText.The test correctly verifies that all expected items are rendered and matches them by label text.
Also applies to: 16-18
45-45: Multiple selection toggles are thoroughly tested.This sequence of clicks and un-toggles is correctly validated, ensuring reliable behavior when toggling items repeatedly.
Also applies to: 47-51, 53-56, 57-57, 59-60
64-65: Single selection logic looks robust.Verifying that only the clicked item is active aligns with the expected single selection.
Also applies to: 66-68, 69-71, 73-75
79-80: Sequential clicks in single selection scenario are handled well.The final check correctly ensures that only the last clicked item remains active.
Also applies to: 81-85, 87-91, 93-93
97-98: Color attribute test is valid.Confirming that the
data-accent-colorattribute is set to "blue" verifies the stylistic prop usage.src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (3)
1-1: Importing React and useState is appropriate.No concerns here; importing useState is essential for local state management.
5-5: RovingFocusGroup import is properly introduced.Replacing manual focus control with a dedicated utility is a beneficial step for maintainability and accessibility.
29-36: Focus management refactor using RovingFocusGroup is well-structured.Wrapping children with
RovingFocusGroup.RootandRovingFocusGroup.Groupcleanly replaces manual handling. It simplifies the component implementation and should help ensure consistent focus behavior.
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 (5)
src/components/ui/Toggle/Toggle.tsx(1 hunks)src/components/ui/ToggleGroup/ToggleGroup.tsx(2 hunks)src/components/ui/ToggleGroup/contexts/toggleContext.tsx(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/ToggleGroup/contexts/toggleContext.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/ui/Toggle/Toggle.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2025-03-13T06:35:04.614Z
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 Definitions (3)
src/components/ui/Toggle/Toggle.tsx (1)
src/core/hooks/useControllableState/index.tsx (1) (1)
useControllableState(15-78)
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1) (1)
ToggleContext(20-24)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1) (1)
ToggleContext(20-24)
🪛 Biome (1.9.4)
src/components/ui/ToggleGroup/ToggleGroup.tsx
[error] 12-12: Shouldn't redeclare 'ToggleItem'. Consider to delete it or rename it.
'ToggleItem' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (23)
src/components/ui/Toggle/Toggle.tsx (6)
4-4: Good addition of useControllableState hookAdding the
useControllableStatehook is a great improvement to support both controlled and uncontrolled component patterns in a standardized way.
10-30: Well-documented props with improved type definitionsThe updated JSDoc comments and type definitions provide clear documentation for both controlled and uncontrolled usage patterns. Making
pressedoptional while addingdefaultPressedfollows React's established pattern for controllable components.
33-47: Great component documentation with usage examplesThe documentation now clearly illustrates both controlled and uncontrolled usage patterns with concrete examples, making it much easier for developers to understand how to use this component correctly.
58-63: Excellent implementation of useControllableStateThe implementation correctly uses the
useControllableStatehook to handle the component's state management in both controlled and uncontrolled modes. This simplifies the component logic and makes it more robust.
65-67: Appropriately removed redundant validationGood removal of the validation logic since the hook now handles the validation and provides appropriate warnings in development mode.
80-80: Simplified event handlingDirectly passing
setIsPressedtoonPressedChangeis cleaner than having an intermediate handler function. This change reduces code complexity while maintaining the same functionality.src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (7)
5-5: Good use of RovingFocusGroup for accessibilityAdding RovingFocusGroup significantly improves keyboard navigation and accessibility without requiring manual focus management code.
7-18: Improved prop type definitions and documentationThe enhanced type definitions with descriptive JSDoc comments make the API more self-documenting and easier to use correctly.
20-33: Comprehensive component documentationAdding detailed JSDoc comments with usage examples significantly improves the developer experience by making it clear how to use this component within the ToggleGroup context.
35-35: Simplified context usageThe code is now more focused on component logic by removing type assertions and directly destructuring the context values.
38-40: Good typing for attribute objectsUsing
Record<string, string>for the attribute objects provides proper typing while maintaining flexibility.
70-77: Improved code commentsAdding a descriptive comment before setting ARIA attributes helps explain the purpose of this code block.
79-86: Simplified component rendering with RovingFocusGroupThe component structure is now cleaner and more focused on its core functionality by delegating focus management to RovingFocusGroup.Item. This also removes a significant amount of manual event handling code.
src/components/ui/ToggleGroup/ToggleGroup.tsx (5)
19-31: Good prop interface definitionThe
ToggleGroupPropstype clearly defines the available props with descriptive comments for each property.
32-40: Well-structured component type definitionThe
ToggleGroupComponenttype properly defines the component with its static properties, making TypeScript aware of the compound component pattern.
41-68: Excellent component documentation with examplesThe comprehensive JSDoc comments with usage examples for both single and multiple selection modes make it clear how to use the component correctly.
69-83: Implementation aligned with type definitionsThe component implementation correctly uses the newly defined types and provides default values for optional props.
87-95: Good documentation for compound componentsAdding descriptive comments for the Root and Item static properties helps developers understand the purpose of each sub-component.
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (5)
5-5: Good use of RovingFocusGroup for accessibilityImporting and utilizing RovingFocusGroup improves keyboard navigation and accessibility without requiring complex manual focus management code.
9-33: Well-structured interface with comprehensive documentationThe
ToggleGroupRootPropsinterface clearly defines all the component's props with descriptive JSDoc comments, improving type safety and developer experience.
34-47: Excellent component documentation with exampleThe JSDoc comments provide a clear explanation of the component's purpose and usage, including a concrete example that demonstrates proper implementation.
48-58: Props destructuring with proper typingThe component now correctly specifies its prop type and provides sensible defaults for optional props, including the new
directionprop.
78-84: Improved component structure with RovingFocusGroupThe component now properly uses RovingFocusGroup to handle keyboard navigation, simplifying the code by removing manual focus management. The nested structure with Root and Group components follows the RovingFocusGroup API correctly.
| /** | ||
| * ToggleGroup Item interface | ||
| * @typedef ToggleItem | ||
| */ | ||
| type ToggleItem = { | ||
| /** Unique value for the toggle item */ | ||
| value: any; | ||
| /** Display label for the toggle item */ | ||
| label: any; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Type name conflict with imported component
The ToggleItem type is named the same as the imported component, which could lead to confusion and is flagged by static analysis.
Rename the type to avoid confusion:
-type ToggleItem = {
+type ToggleItemType = {
/** Unique value for the toggle item */
value: any;
/** Display label for the toggle item */
label: any;
};Then update the items prop type in ToggleGroupProps:
- items?: ToggleItem[];
+ items?: ToggleItemType[];📝 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.
| /** | |
| * ToggleGroup Item interface | |
| * @typedef ToggleItem | |
| */ | |
| type ToggleItem = { | |
| /** Unique value for the toggle item */ | |
| value: any; | |
| /** Display label for the toggle item */ | |
| label: any; | |
| }; | |
| /** | |
| * ToggleGroup Item interface | |
| * @typedef ToggleItemType | |
| */ | |
| -type ToggleItem = { | |
| +type ToggleItemType = { | |
| /** Unique value for the toggle item */ | |
| value: any; | |
| /** Display label for the toggle item */ | |
| label: any; | |
| }; | |
| // ... other parts of the file ... | |
| type ToggleGroupProps = { | |
| // ... other properties ... | |
| - items?: ToggleItem[]; | |
| + items?: ToggleItemType[]; | |
| // ... other properties ... | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Shouldn't redeclare 'ToggleItem'. Consider to delete it or rename it.
'ToggleItem' is defined here:
(lint/suspicious/noRedeclare)
1 participant
Summary by CodeRabbit
New Features
useControllableState, for managing controlled and uncontrolled component states effectively.Bug Fixes