Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new Toggle component to the raystack design system, including the React component implementation with CSS styling, size variants, a nested Toggle.Group subcomponent, comprehensive test coverage, documentation, and playground examples. The component is exported at the package level. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/raystack/components/toggle/toggle.tsx (1)
24-28: Redundant size type declaration.The
size?: 1 | 2 | 3 | 4declaration on line 27 duplicates whatVariantProps<typeof toggleVariants>already infers from the CVA definition. This redundancy could lead to maintenance issues if sizes change.♻️ Suggested simplification
export interface ToggleProps extends TogglePrimitive.Props, - VariantProps<typeof toggleVariants> { - size?: 1 | 2 | 3 | 4; -} + VariantProps<typeof toggleVariants> {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/toggle/toggle.tsx` around lines 24 - 28, Remove the redundant explicit size declaration from the ToggleProps interface: rely on VariantProps<typeof toggleVariants> to provide the size type instead of the separate "size?: 1 | 2 | 3 | 4" line; edit the ToggleProps declaration (symbol: ToggleProps) to extend TogglePrimitive.Props and VariantProps<typeof toggleVariants> only, remove the "size" property, and run typecheck to ensure no other code expects the removed literal type and update those callsites if necessary.apps/www/src/content/docs/components/toggle/demo.ts (2)
1-9: Consider adding type annotation for props parameter.Using
anyfor thepropsparameter loses type safety. A more specific type would improve maintainability.♻️ Suggested improvement
-const getCode = (props: any) => { +const getCode = (props: Record<string, unknown>) => { return `<Toggle aria-label="Bold"${getPropsString(props)}> <FontBoldIcon /> </Toggle>`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/toggle/demo.ts` around lines 1 - 9, The getCode function currently types its props parameter as any; change it to a specific props type (e.g., React.ComponentProps<typeof Toggle> or a dedicated ToggleProps/Partial<ToggleProps> type imported from the Toggle component) to restore type safety, update the function signature for getCode(props: /* new type */) and adjust any callers or the getPropsString helper signature if necessary so types align with getPropsString(props) and the <Toggle ...> usage.
68-82: Controlled example's callback signature differs from documentation.The
controlledDemopassessetPresseddirectly toonPressedChange, but per the documentedToggleProps, the callback signature is(pressed: boolean, event: Event) => void. While React'suseStatesetter will ignore the extraeventargument at runtime, this example may confuse users expecting to use the event parameter.Consider showing the full signature for educational clarity:
📝 Suggested clarification
export const controlledDemo = { type: 'code', code: `function ControlledToggle() { const [pressed, setPressed] = React.useState(false); return ( <Toggle aria-label="Star" pressed={pressed} - onPressedChange={setPressed} + onPressedChange={(newPressed) => setPressed(newPressed)} > {pressed ? "★" : "☆"} </Toggle> ); }` };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/toggle/demo.ts` around lines 68 - 82, The controlled example passes setPressed directly to onPressedChange which hides the documented callback signature (pressed: boolean, event: Event) — update the ControlledToggle example (controlledDemo) to accept both parameters in the handler: define an onPressedChange wrapper (e.g., (pressed, event) => setPressed(pressed)) and use that instead of passing setPressed directly so the example matches ToggleProps and shows the event parameter; optionally add a brief param name or type annotation to the handler to make the signature explicit.packages/raystack/components/toggle/__tests__/toggle.test.tsx (1)
48-84: Consider adding size={2} test for completeness.The size variants tests cover sizes 1, 3, and 4 but skip size 2. While unlikely to fail given the pattern, adding it would ensure full coverage.
💚 Suggested addition
it('applies size 1 class', () => { render( <Toggle aria-label='Bold' size={1}> B </Toggle> ); const toggle = screen.getByRole('button', { name: 'Bold' }); expect(toggle).toHaveClass(styles['size-1']); }); + it('applies size 2 class', () => { + render( + <Toggle aria-label='Bold' size={2}> + B + </Toggle> + ); + const toggle = screen.getByRole('button', { name: 'Bold' }); + expect(toggle).toHaveClass(styles['size-2']); + }); + it('applies size 3 class', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/toggle/__tests__/toggle.test.tsx` around lines 48 - 84, Add a missing unit test for the size=2 variant inside the "Size Variants" describe block by mirroring the existing size tests: render <Toggle aria-label='Bold' size={2}>B</Toggle>, query the button with screen.getByRole('button', { name: 'Bold' }) and assert it has the styles['size-2'] class; place the new it('applies size 2 class', ...) alongside the other size tests so Toggle and styles['size-2'] are exercised for full coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/components/playground/toggle-examples.tsx`:
- Around line 37-48: The four icon-only Toggle examples (Toggle components
containing FontBoldIcon) all use the same aria-label='Bold', making the size
demo indistinguishable to screen readers; update each Toggle instance in
toggle-examples.tsx to provide a unique accessible name (e.g., aria-label='Bold
size 1', 'Bold size 2', etc.) or add per-sample visible size text or
visually-hidden labels next to the FontBoldIcon so each sample is announced
distinctly by assistive technology.
In `@apps/www/src/content/docs/components/toggle/demo.ts`:
- Around line 11-29: The playground controls define size options as strings
which leads to size being emitted as a string prop; update the
playground.controls.size definition in the demo so options are numeric (e.g.,
[1, 2, 3, 4]) and set defaultValue to a number (e.g., 3) so
getCode/getPropsString will generate size={3}; keep the rest of the controls and
getCode reference unchanged.
In `@packages/raystack/components/toggle/toggle.module.css`:
- Around line 14-15: The inner .toggleContent keeps a standalone border-radius
which makes grouped toggles render as rounded pills; update the CSS so
.toggleContent has border-radius: 0 when it lives inside a grouped toggle
container (i.e., when its parent .toggle is part of a group), and re-apply
rounded corners only for the group's first and last items using selectors like
.toggleGroup .toggle:first-child .toggleContent and .toggleGroup
.toggle:last-child .toggleContent (or the existing group modifier class you
use), and apply the same reset to the other occurrences referenced (lines ~31-33
and 75-89) so hover/pressed fills render flush across the segmented surface.
- Around line 27-40: The hover/pressed styles should be limited to enabled
toggles instead of disabling pointer events; update the selectors from
.toggle:hover and .toggle[data-pressed] (and their .toggleContent counterparts)
to guard with :not([data-disabled]) (e.g., .toggle:not([data-disabled]):hover
and .toggle:not([data-disabled])[data-pressed] and likewise for .toggleContent)
and remove pointer-events: none from .toggle[data-disabled] so cursor:
not-allowed remains effective while keeping cursor and opacity.
---
Nitpick comments:
In `@apps/www/src/content/docs/components/toggle/demo.ts`:
- Around line 1-9: The getCode function currently types its props parameter as
any; change it to a specific props type (e.g., React.ComponentProps<typeof
Toggle> or a dedicated ToggleProps/Partial<ToggleProps> type imported from the
Toggle component) to restore type safety, update the function signature for
getCode(props: /* new type */) and adjust any callers or the getPropsString
helper signature if necessary so types align with getPropsString(props) and the
<Toggle ...> usage.
- Around line 68-82: The controlled example passes setPressed directly to
onPressedChange which hides the documented callback signature (pressed: boolean,
event: Event) — update the ControlledToggle example (controlledDemo) to accept
both parameters in the handler: define an onPressedChange wrapper (e.g.,
(pressed, event) => setPressed(pressed)) and use that instead of passing
setPressed directly so the example matches ToggleProps and shows the event
parameter; optionally add a brief param name or type annotation to the handler
to make the signature explicit.
In `@packages/raystack/components/toggle/__tests__/toggle.test.tsx`:
- Around line 48-84: Add a missing unit test for the size=2 variant inside the
"Size Variants" describe block by mirroring the existing size tests: render
<Toggle aria-label='Bold' size={2}>B</Toggle>, query the button with
screen.getByRole('button', { name: 'Bold' }) and assert it has the
styles['size-2'] class; place the new it('applies size 2 class', ...) alongside
the other size tests so Toggle and styles['size-2'] are exercised for full
coverage.
In `@packages/raystack/components/toggle/toggle.tsx`:
- Around line 24-28: Remove the redundant explicit size declaration from the
ToggleProps interface: rely on VariantProps<typeof toggleVariants> to provide
the size type instead of the separate "size?: 1 | 2 | 3 | 4" line; edit the
ToggleProps declaration (symbol: ToggleProps) to extend TogglePrimitive.Props
and VariantProps<typeof toggleVariants> only, remove the "size" property, and
run typecheck to ensure no other code expects the removed literal type and
update those callsites if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a75733d-55ee-4e57-a761-c95079cb1ab0
📒 Files selected for processing (12)
apps/www/src/components/demo/demo.tsxapps/www/src/components/playground/index.tsapps/www/src/components/playground/toggle-examples.tsxapps/www/src/content/docs/components/toggle/demo.tsapps/www/src/content/docs/components/toggle/index.mdxapps/www/src/content/docs/components/toggle/props.tspackages/raystack/components/icon-button/icon-button.tsxpackages/raystack/components/toggle/__tests__/toggle.test.tsxpackages/raystack/components/toggle/index.tspackages/raystack/components/toggle/toggle.module.csspackages/raystack/components/toggle/toggle.tsxpackages/raystack/index.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ToggleandToggle.Groupcomponents wrapping Base UI's Toggle and ToggleGroup primitivesTogglesupportssizeprop (1–4) via CVA, matching IconButton sizingToggle.Groupprovides shared pressed state with single or multiple selection, with grouped styling (border-radius handled viaoverflow: clip)Summary by CodeRabbit
New Features
Documentation
Tests