feat: forward refs in toggle components #1453
Conversation
WalkthroughRefactors Toggle, ToggleGroupRoot, ToggleItem, and TogglePrimitive to forward refs via React.forwardRef with updated prop typings. Updates Storybook stories (Disclosure to CSF v3 typing; Toggle story callback param annotations). Adds tests verifying ref forwarding for Toggle, ToggleGroup components, and TogglePrimitive. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User Code
participant T as Toggle (component)
participant TP as TogglePrimitive
participant BP as ButtonPrimitive
U->>T: render <Toggle ref={r} pressed/onPressedChange />
T->>TP: render {...props} ref={r}
TP->>BP: render with aria/data attrs ref={r}
Note over BP: r resolves to HTMLButtonElement
U-->>BP: Click/Key events
BP-->>TP: onClick/onKeyDown
TP-->>U: onPressedChange(isPressed)
sequenceDiagram
autonumber
participant U as User Code
participant R as ToggleGroupRoot
participant G as RovingFocusGroup (asChild)
participant D as div (root)
participant I as ToggleItem
participant TIP as TogglePrimitive
U->>R: render <Root ref={rootRef} roving>
R->>G: render asChild
G->>D: render div ref={rootRef}
U->>I: render <Item ref={itemRef} value=...>
I->>TIP: render with ref={itemRef}
Note over D,I: rootRef -> HTMLDivElement<br/>itemRef -> HTMLButtonElement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (
|
|
closing as stale |
kotAPI
deleted the
kotapi/refactor-toggle-component-to-use-forwardref
branch
![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 (5)
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (3)
96-106: Ref is forwarded to the wrong element; also relying on click instead of pressed-change.The ref is attached to
RovingFocusGroup.Item, not the underlying button, andonClickmakesTogglePrimitivemanage its own state, risking divergence from group state.Forward via
asChildand control the primitive:- return <RovingFocusGroup.Item ref={ref as React.Ref<HTMLButtonElement>}> - <TogglePrimitive - onClick={handleToggleSelect} - className={`${rootClass}-item ${className}`} - disabled={isDisabled} - asChild={asChild} - {...ariaProps} - {...dataProps} - {...props} - >{children}</TogglePrimitive> - </RovingFocusGroup.Item>; + return ( + <RovingFocusGroup.Item asChild> + <TogglePrimitive + ref={ref} + pressed={isActive} + onPressedChange={handlePressedChange} + className={`${rootClass}-item ${className}`} + disabled={isDisabled} + asChild={asChild} + {...props} + > + {children} + </TogglePrimitive> + </RovingFocusGroup.Item> + );
52-80: Drive selection via onPressedChange (single/multiple) instead of onClick.Replace the click handler with a pressed-change handler to keep group and item visuals in sync.
- const handleToggleSelect = () => { - if (isDisabled) { - return; - } - - let activeToggleArray = activeToggles || []; - - // For Single Case - if (type === 'single') { - if (isActive) { - setActiveToggles([]); - return; - } else { - setActiveToggles([value]); - return; - } - } - - // For Multiple Case - if (type === 'multiple') { - if (isActive) { - activeToggleArray = activeToggleArray.filter((item: any) => item !== value); - } else { - activeToggleArray = [...activeToggleArray, value]; // Using spread operator to create a new array - } - } - - setActiveToggles(activeToggleArray); - }; + const handlePressedChange = (next: boolean) => { + if (isDisabled) return; + const prev = activeToggles ?? []; + if (type === 'single') { + setActiveToggles(next ? [value] : []); + return; + } + // multiple + if (next) { + setActiveToggles(isActive ? prev : [...prev, value]); + } else { + setActiveToggles(prev.filter((item: any) => item !== value)); + } + };
46-95: Control TogglePrimitive instead of manual aria/data overrides
Passpressed={isActive}andonPressedChange={handleToggleSelect}to TogglePrimitive, then remove theariaProps/dataPropslogic and their spreads.src/core/primitives/Toggle/index.tsx (2)
19-28: Fix:labelis destructured but not declared onTogglePrimitiveProps.This will fail type-checking if
labelis no longer part of the API. Remove it and rely on consumers to passaria-labeldirectly.Apply:
-const TogglePrimitive = forwardRef<TogglePrimitiveElement, TogglePrimitiveProps>(({ - children, - label = '', - defaultPressed = false, - pressed: controlledPressed, - onPressedChange = () => {}, - disabled, - asChild = false, - ...props -}, ref) => { +const TogglePrimitive = forwardRef<TogglePrimitiveElement, TogglePrimitiveProps>(({ + children, + defaultPressed = false, + pressed: controlledPressed, + onPressedChange = () => {}, + disabled, + asChild = false, + ...props +}, ref) => {
57-67: Make computed aria attributes authoritative (including aria-label) and drop theariaAttributesspread.Place
{...props}before the computed ARIA attributes, manually spreadaria-labeliflabelis provided, and use booleanaria-pressed/aria-disabled. The event handler utility already respectsevent.defaultPrevented.return <ButtonPrimitive - ref={ref} - onClick={composeEventHandlers(props.onClick, handleTogglePressed)} - onKeyDown={composeEventHandlers(props.onKeyDown, handleKeyDown)} - data-state={isPressed ? 'on' : 'off'} - data-disabled={disabled ? '' : undefined} - disabled={disabled} - asChild={asChild} - {...ariaAttributes} - {...props} + ref={ref} + asChild={asChild} + disabled={disabled} + data-state={isPressed ? 'on' : 'off'} + data-disabled={disabled ? '' : undefined} + {...props} + {...(label ? { 'aria-label': label } : {})} + aria-pressed={isPressed} + aria-disabled={disabled} onClick={composeEventHandlers(props.onClick, handleTogglePressed)} onKeyDown={composeEventHandlers(props.onKeyDown, handleKeyDown)} > {children}Ensure existing stories relying on
label→aria-labelstill work.
🧹 Nitpick comments (12)
src/components/ui/Disclosure/stories/Disclosure.stories.tsx (3)
2-2: Import StoryObj to get typed stories (type-only import).This enables typing for story objects and keeps the import type-only.
-import type { Meta } from '@storybook/react'; +import type { Meta, StoryObj } from '@storybook/react';If you adopt this, consider typing your stories:
type Story = StoryObj<typeof meta>; // Example — keep your existing args as-is export const All: Story = { args: {/* existing args */} }; // Optional: convert function story to object form export const Composed: Story = { render: () => (/* existing JSX */) };
6-16: Prefersatisfies Meta<typeof Disclosure>to preserve inference and catch excess keys.This keeps strong type-checking without widening literal types.
-const meta: Meta<typeof Disclosure> = { +const meta = { title: 'WIP/Disclosure', component: Disclosure, - render: (args: JSX.IntrinsicAttributes & DisclosureProps) => ( + render: (args) => ( <SandboxEditor> <div> <Disclosure {...args} /> </div> </SandboxEditor> ) -}; +} satisfies Meta<typeof Disclosure>;
9-15: Don’t annotate storyargswithJSX.IntrinsicAttributes; rely on inference orComponentProps.
JSX.IntrinsicAttributesis unnecessary here and can mask prop typing. Let Meta inference handle it (or useReact.ComponentProps<typeof Disclosure>if you need explicit typing).- render: (args: JSX.IntrinsicAttributes & DisclosureProps) => ( + render: (args) => (If you keep the change above, you can also drop the now-unused
DisclosurePropsandJSXimports:// imports (outside selected range) import React from 'react'; // remove: { JSX } // remove: { DisclosureProps }Optional alternative: prefer a decorator instead of
meta.renderto wrap all stories uniformly (helps if you later add more stories):// Replace render with: decorators: [(Story) => ( <SandboxEditor> <div><Story /></div> </SandboxEditor> )],src/components/ui/Toggle/tests/Toggle.test.js (1)
11-15: Prefer Jest's native matcher for instance checksUse
toBeInstanceOffor better failure messages and readability.- expect(ref.current instanceof HTMLButtonElement).toBe(true); + expect(ref.current).toBeInstanceOf(HTMLButtonElement);src/components/ui/ToggleGroup/tests/ToggleGroup.test.js (2)
21-29: Prefer Jest's native matcher for instance checks (Root ref)Use
toBeInstanceOffor clearer intent and diagnostics.- expect(ref.current instanceof HTMLDivElement).toBe(true); + expect(ref.current).toBeInstanceOf(HTMLDivElement);
31-39: Prefer Jest's native matcher for instance checks (Item ref)Use
toBeInstanceOffor consistency with other tests.- expect(ref.current instanceof HTMLButtonElement).toBe(true); + expect(ref.current).toBeInstanceOf(HTMLButtonElement);src/components/ui/Toggle/Toggle.tsx (2)
59-63: Use camelCase for local variablesMinor consistency nit: prefer
dataAttributesoverdata_attributes.- const data_attributes: Record<string, string> = {}; + const dataAttributes: Record<string, string> = {}; - if (color) { - data_attributes['data-rad-ui-accent-color'] = color; - } + if (color) { + dataAttributes['data-rad-ui-accent-color'] = color; + }
65-76: Ensure computed props win over consumer overridesSpread
...propsbefore computed attributes sopressed,onPressedChange,data-state, anddata-disabledcan’t be accidentally overridden by consumer-supplied props (e.g., customdata-state). No behavior change intended.return ( <TogglePrimitive - ref={ref} - className={clsx(rootClass, className)} - pressed={isPressed} - onPressedChange={setIsPressed} - data-state={isPressed ? 'on' : 'off'} - data-disabled={props.disabled ? '' : undefined} - asChild={asChild} - {...props} - {...data_attributes} + ref={ref} + {...props} + className={clsx(rootClass, className)} + asChild={asChild} + pressed={isPressed} + onPressedChange={setIsPressed} + data-state={isPressed ? 'on' : 'off'} + data-disabled={props.disabled ? '' : undefined} + {...dataAttributes} >src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (2)
41-41:asChildprop on ToggleGroupRoot is declared but unused.Either wire it to actually render the root via a Slot when
asChildis true, or drop the prop to avoid API confusion. Right now it does nothing.Also applies to: 62-64
74-81: Memoize the context value to avoid unnecessary re-renders.
sendValuesis recreated on every render, forcing all consumers to re-render.Apply:
- const sendValues = { - activeToggles, - setActiveToggles, - type, - rootClass, - disabled, - orientation - }; + const sendValues = React.useMemo(() => ({ + activeToggles, + setActiveToggles, + type, + rootClass, + disabled, + orientation + }), [activeToggles, type, rootClass, disabled, orientation]);src/core/primitives/Toggle/index.tsx (2)
46-51: Type the keyboard event.Use a concrete React type; safer for
asChildcases.- const handleKeyDown = (event: any) => { + const handleKeyDown = (event: React.KeyboardEvent<HTMLElement>) => {
53-56: Remove ad-hocariaAttributesand theanytyping.Avoid stringifying booleans and prefer explicit aria props; we’ll set them directly on the element.
- const ariaAttributes:any = label ? { 'aria-label': label } : {}; - ariaAttributes['aria-pressed'] = isPressed ? 'true' : 'false'; - ariaAttributes['aria-disabled'] = disabled ? 'true' : 'false';
📜 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 (9)
src/components/ui/Disclosure/stories/Disclosure.stories.tsx(1 hunks)src/components/ui/Toggle/Toggle.tsx(6 hunks)src/components/ui/Toggle/stories/Toggle.stories.tsx(4 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(6 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx(4 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(1 hunks)src/core/primitives/Toggle/index.tsx(4 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/core/primitives/Toggle/tests/TogglePrimitive.test.tsxsrc/components/ui/Toggle/stories/Toggle.stories.tsxsrc/components/ui/ToggleGroup/fragments/ToggleItem.tsxsrc/components/ui/Toggle/Toggle.tsxsrc/core/primitives/Toggle/index.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
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`.
Applied to files:
src/components/ui/Toggle/stories/Toggle.stories.tsx
🧬 Code graph analysis (2)
src/components/ui/Disclosure/stories/Disclosure.stories.tsx (1)
src/components/ui/Disclosure/Disclosure.tsx (1)
DisclosureProps(7-9)
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
src/components/ui/ToggleGroup/contexts/toggleContext.tsx (1)
ToggleContext(26-33)
⏰ 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 (9)
src/components/ui/Disclosure/stories/Disclosure.stories.tsx (1)
18-18: LGTM on default export.The CSF v3 default export of the typed
metalooks good.src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx (1)
11-15: LGTM: correct ref-forwarding assertionUses
toBeInstanceOf(HTMLButtonElement)and covers the core behavior well.src/components/ui/Toggle/Toggle.tsx (1)
36-47: LGTM: forwardRef conversion and typingThe
forwardRef<ToggleElement, ToggleProps>pattern and prop extraction look solid.src/components/ui/Toggle/stories/Toggle.stories.tsx (1)
126-126: Typed onPressedChange params align with the updated API.Explicit boolean annotations match the new
(isPressed: boolean) => voidsignature and improve story clarity. LGTM.Also applies to: 186-186, 195-195, 204-204
src/core/primitives/Toggle/index.tsx (5)
1-6: Imports — LGTM.Good addition of forwardRef, ElementRef, and ComponentPropsWithoutRef.
11-17: Public types look good.Extending ButtonPrimitive props is appropriate; custom toggle props are clearly scoped.
31-44: Controlled/uncontrolled state handling — LGTM.Matches our prior decision not to sync internal state when controlled (see retrieved learning).
58-58: Ref forwarding — LGTM.Ref correctly lands on ButtonPrimitive; tests should cover this.
71-71: displayName — LGTM.Helpful for DevTools.
| <RovingFocusGroup.Group | ||
| className={clsx(rootClass, className)} | ||
| {...data_attributes} | ||
| {...({ asChild: true } as any)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the as any escape hatch on RovingFocusGroup.Group.
Leaning on as any hides type issues and weakens safety. Prefer proper typing support for asChild on RovingFocusGroup.Group, then pass it directly.
Apply this diff here (and fix Group’s prop types accordingly):
- {...({ asChild: true } as any)}
+ asChild📝 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.
| {...({ asChild: true } as any)} | |
| asChild |
Summary
Testing
npm testnpm run build:rollupSummary by CodeRabbit