refactor(toggle-group): forward refs #1470
Conversation
|
Warning Rate limit exceeded@kotAPI has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors ToggleGroup to a forwardRef-based default export with static Root and Item members, updates fragments to use React.forwardRef, adds defaulted props to ToggleItem, and introduces an accessibility test verifying aria-pressed toggling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as App Code
participant TG as ToggleGroup (default)
participant R as ToggleGroup.Root
participant I as ToggleGroup.Item (button)
C->>TG: Import default with .Root and .Item
C->>R: Render group container
R->>I: Render toggle item
U->>I: Click
I->>I: Toggle pressed state (aria-pressed false -> true)
I-->>U: Updated ARIA reflects state
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
108-125: Bug: props/data-attributes are applied to RovingFocusGroup.Group, not the DOM node receiving the ref.When rovingFocus=true, {...props} and {...data_attributes} land on RovingFocusGroup.Group instead of the div that holds ref. Consumers expecting id/aria-* on the actual root DOM will see a regression. Move spreads onto the div for parity with the non-roving branch.
Apply:
- <RovingFocusGroup.Group - {...data_attributes} - {...props} - > - <div ref={ref} className={clsx(rootClass, className)}> + <RovingFocusGroup.Group> + <div + ref={ref} + className={clsx(rootClass, className)} + {...data_attributes} + {...props} + > {children} </div> </RovingFocusGroup.Group>src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
39-41: Guard against non-array controlled values.If Root ever provides a scalar in 'single' mode, String.prototype.includes semantics would be wrong. Coerce to an array for comparisons and reuse it for mutations.
Apply:
- const { type, activeToggles, setActiveToggles, rootClass, disabled: groupDisabled } = useContext(ToggleContext); - const isActive = activeToggles?.includes(value); + const { type, activeToggles, setActiveToggles, rootClass, disabled: groupDisabled } = useContext(ToggleContext); + const currentValues = Array.isArray(activeToggles) + ? activeToggles + : activeToggles == null + ? [] + : [activeToggles]; + const isActive = currentValues.includes(value as any); @@ - let activeToggleArray = activeToggles || []; + let activeToggleArray = currentValues;Also applies to: 56-56
🧹 Nitpick comments (6)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (4)
57-57: Unused prop: asChild on Root.Root accepts asChild but never uses it. Either remove it or wire it via a Slot to actually render-as-child.
64-77: Clarify and enforce the value shape (single vs multiple).State is handled as arrays (includes/filter) regardless of type. If public API allows a scalar for 'single', this will be inconsistent. Either:
- enforce arrays for both modes and document it, or
- model conditional types and store scalar for 'single' and array for 'multiple'.
Do you want me to draft types and a minimal adapter to normalize values in context?
42-42: Improve displayName for DevTools clarity.Root currently reports as "ToggleGroup". Consider "ToggleGroup.Root" to disambiguate from the default export.
Example:
-const COMPONENT_NAME = 'ToggleGroup'; +const COMPONENT_NAME = 'ToggleGroup.Root';Also applies to: 128-128
89-89: Unnecessary cast.orientation is a string union; casting to any for data attribute isn’t needed.
- data_attributes['data-orientation'] = orientation as any; + data_attributes['data-orientation'] = orientation;src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
45-47: Type ARIA props correctly and pass booleans.Use React.AriaAttributes and boolean for aria-pressed.
- const ariaProps:Record<string, string> = {}; + const ariaProps: React.AriaAttributes = {}; @@ - if (isActive) { - ariaProps['aria-pressed'] = 'true'; + if (isActive) { + ariaProps['aria-pressed'] = true; dataProps['data-state'] = 'on'; } else { - ariaProps['aria-pressed'] = 'false'; + ariaProps['aria-pressed'] = false; dataProps['data-state'] = 'off'; }Also applies to: 81-88
src/components/ui/ToggleGroup/tests/ToggleGroup.test.tsx (1)
5-24: Add a ref-behavior assertion for Root props propagation.Once Root moves {...props} onto the ref’d div (see Root review), add a test to assert that custom attributes (e.g., id, data-*) end up on the same element as the forwarded ref in both rovingFocus modes.
Happy to add that test in this PR.
📜 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 (4)
src/components/ui/ToggleGroup/ToggleGroup.tsx(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx(2 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-24T06:43:42.194Z
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.
Applied to files:
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx
🪛 GitHub Check: lint
src/components/ui/ToggleGroup/ToggleGroup.tsx
[failure] 15-15:
Component definition is missing display name
🪛 GitHub Actions: Lint
src/components/ui/ToggleGroup/ToggleGroup.tsx
[error] 15-15: ESLint: Component definition is missing display name. (react/display-name)
⏰ 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/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
44-60: Nice forwardRef migration.Ref is correctly forwarded to the DOM element; defaults look sensible.
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
31-38: ForwardRef adoption looks good.Signature and defaults align with the new API.
src/components/ui/ToggleGroup/tests/ToggleGroup.test.tsx (2)
40-50: Good a11y assertion.aria-pressed transitions are verified end-to-end.
26-38: Solid smoke test for warnings.Covers regressions from prop-types/usage; thanks for guarding console noise.
src/components/ui/ToggleGroup/ToggleGroup.tsx (1)
22-25: Static members wiring looks correct.Root and Item are attached after the cast; matches intended public API.
| const ToggleGroup = React.forwardRef<ToggleGroupElement, ToggleGroupProps>((_props, _ref) => { | ||
| console.warn( | ||
| 'Direct usage of ToggleGroup is not supported. Please use ToggleGroup.Root, ToggleGroup.Item, etc. instead.' | ||
| ); | ||
| return null; | ||
| }) as ToggleGroupComponent; | ||
|
|
There was a problem hiding this comment.
Fix lint: missing displayName (react/display-name).
Assign displayName to the forwardRef component. Also gate the console.warn to dev-only.
Apply:
-const ToggleGroup = React.forwardRef<ToggleGroupElement, ToggleGroupProps>((_props, _ref) => {
- console.warn(
- 'Direct usage of ToggleGroup is not supported. Please use ToggleGroup.Root, ToggleGroup.Item, etc. instead.'
- );
- return null;
-}) as ToggleGroupComponent;
+const ToggleGroup = React.forwardRef<ToggleGroupElement, ToggleGroupProps>((_props, _ref) => {
+ if (process.env.NODE_ENV !== 'production') {
+ console.warn(
+ 'Direct usage of ToggleGroup is not supported. Please use ToggleGroup.Root, ToggleGroup.Item, etc. instead.'
+ );
+ }
+ return null;
+}) as ToggleGroupComponent;
+
+ToggleGroup.displayName = 'ToggleGroup';📝 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 ToggleGroup = React.forwardRef<ToggleGroupElement, ToggleGroupProps>((_props, _ref) => { | |
| console.warn( | |
| 'Direct usage of ToggleGroup is not supported. Please use ToggleGroup.Root, ToggleGroup.Item, etc. instead.' | |
| ); | |
| return null; | |
| }) as ToggleGroupComponent; | |
| const ToggleGroup = React.forwardRef<ToggleGroupElement, ToggleGroupProps>((_props, _ref) => { | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.warn( | |
| 'Direct usage of ToggleGroup is not supported. Please use ToggleGroup.Root, ToggleGroup.Item, etc. instead.' | |
| ); | |
| } | |
| return null; | |
| }) as ToggleGroupComponent; | |
| ToggleGroup.displayName = 'ToggleGroup'; |
🧰 Tools
🪛 GitHub Check: lint
[failure] 15-15:
Component definition is missing display name
🪛 GitHub Actions: Lint
[error] 15-15: ESLint: Component definition is missing display name. (react/display-name)
🤖 Prompt for AI Agents
In src/components/ui/ToggleGroup/ToggleGroup.tsx around lines 15 to 21, the
forwardRef component is missing a displayName and prints a console.warn
unguarded; set ToggleGroup.displayName = 'ToggleGroup' after the forwardRef
assignment and wrap the console.warn so it only runs in development (e.g., guard
with process.env.NODE_ENV !== 'production' or a __DEV__ check) to avoid logging
in production.
kotAPI
deleted the
kotapi/refactor-togglegroup-to-use-forwardref-xrdib3
branch
Summary
Testing
npm test src/components/ui/ToggleGroup/tests/ToggleGroup.test.tsxnpm run build:rollupSummary by CodeRabbit
New Features
Accessibility
Tests
Refactor