chore: Update Switch with new designs - BED-7915#2638
Open
KillahDillah wants to merge 3 commits intomainfrom
Open
chore: Update Switch with new designs - BED-7915#2638KillahDillah wants to merge 3 commits intomainfrom
KillahDillah wants to merge 3 commits intomainfrom
Conversation
KillahDillah
added
the
user interface
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved size variants and related props from Switch; replaced variant-driven CVA with fixed classes; label is now conditionally rendered and aria-label applied only when label is absent; root id uses Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 55-79: The label uses switchId but callers can pass an id via
props, so compute the actual id as const switchId = props.id ?? React.useId()
(or extract id from props: const { id, ...rest } = props; const switchId = id ??
React.useId()), use switchId for both SwitchPrimitives.Root id and label
htmlFor, and spread the remaining props (rest) into SwitchPrimitives.Root to
avoid duplicating/overwriting the id; update references to props when needed so
SwitchPrimitives.Root receives id={switchId} and {...rest} while labels use
htmlFor={switchId}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fee75ea9-3a3a-4fcc-9659-8bf3135282f8
📒 Files selected for processing (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.tsx
KillahDillah
changed the title
KillahDillah
force-pushed
the
BED-7915-switch-refactor-pt2
branch
from
9f02407 to
f43a9a4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 34-45: The label styling always includes hover:cursor-pointer and
never responds to disabled; update the LabelVariants cva to add a disabled
variant (e.g., variants: { disabled: { true: 'cursor-default opacity-50
no-underline', false: 'hover:cursor-pointer' } } with default disabled: false)
and remove the unconditional 'hover:cursor-pointer' from the base class, then
ensure wherever LabelVariants is called in Switch/Switch.tsx you pass the
component's disabled prop (e.g., LabelVariants({ position, disabled })) so
labeled, disabled switches render visually non-interactive.
- Around line 55-57: The component currently calls React.useId() conditionally
via "id ?? React.useId()", which violates the Rules of Hooks; change to call
React.useId() unconditionally (e.g., const generatedId = React.useId()) and then
compute switchId = id ?? generatedId so the hook is invoked every render; update
references to "switchId" accordingly in the component (the arrow component body
that declares ariaLabel and switchId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b64df907-0724-4378-bcdf-4347c975f32d
📒 Files selected for processing (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.tsx
KillahDillah
force-pushed
the
BED-7915-switch-refactor-pt2
branch
from
f43a9a4 to
1079115
Compare
KillahDillah
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/javascript/doodle-ui/src/components/Switch/Switch.tsx (1)
34-36: Remove commented-out code.Line 35 contains dead code that should be removed.
Proposed fix
const LabelVariants = cva( - // 'text-base text-neutral-dark-5 transition-all ease-in-out hover:cursor-pointer dark:text-white', 'text-base text-neutral-dark-5 transition-all ease-in-out dark:text-white',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines 34 - 36, Remove the dead commented-out class string inside the LabelVariants cva call by deleting the commented line ('// 'text-base text-neutral-dark-5 transition-all ease-in-out hover:cursor-pointer dark:text-white',) so LabelVariants only contains the active class string; update the cva invocation in the LabelVariants constant to eliminate the comment and keep the remaining class string intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 61-63: The component destructures an id prop but then ignores it
by always creating switchId with React.useId(); update the logic in the Switch
component (the function receiving props and ref) to call React.useId()
unconditionally but prefer the passed-in id when assigning the effective ID
(e.g., set switchId = id || generatedId), and ensure the id is passed to the
rendered input/label so consumers using an explicit id (like
multi-factor-authentication) work and ESLint no-unused-vars errors are resolved.
---
Nitpick comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 34-36: Remove the dead commented-out class string inside the
LabelVariants cva call by deleting the commented line ('// 'text-base
text-neutral-dark-5 transition-all ease-in-out hover:cursor-pointer
dark:text-white',) so LabelVariants only contains the active class string;
update the cva invocation in the LabelVariants constant to eliminate the comment
and keep the remaining class string intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 401e93ad-e7e1-48cd-8e37-d16b23f65975
📒 Files selected for processing (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsx
KillahDillah
force-pushed
the
BED-7915-switch-refactor-pt2
branch
from
1079115 to
4b64c8e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/javascript/doodle-ui/src/components/Switch/Switch.tsx (1)
63-84:⚠️ Potential issue | 🟠 MajorLabel
htmlFordoesn't match rootidwhen caller provides anidprop.The root uses
id={id ?? switchId}(line 75), but both labels usehtmlFor={switchId}(lines 68, 84). When a caller passes an explicitid, the root gets that id while labels still point to the generated id—breaking the label/input association.Compute the effective ID once and use it consistently:
🐛 Proposed fix
({ className, disabled, id, label, labelPosition = 'right', ...props }, ref) => { const ariaLabel = label || 'switch'; - const switchId = React.useId(); + const generatedId = React.useId(); + const switchId = id ?? generatedId; return ( <div className={switchWrapperStyles()}> {labelPosition === 'left' && label && ( <label className={LabelVariants({ position: 'left', disabled })} htmlFor={switchId}> {label} </label> )} <SwitchPrimitives.Root ref={ref} - id={id ?? switchId} + id={switchId} aria-label={!label ? ariaLabel : undefined} className={cn(switchRootStyles(), className)} disabled={disabled} {...props}> <SwitchPrimitives.Thumb className={switchThumbStyles()} /> </SwitchPrimitives.Root> {labelPosition !== 'left' && label && ( <label className={LabelVariants({ position: 'right', disabled })} htmlFor={switchId}> {label} </label> )} </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines 63 - 84, The label htmlFor values currently always use the generated switchId while the root id uses id ?? switchId, breaking association when a caller passes id; compute a single effectiveId (e.g., const effectiveId = id ?? switchId) and use that for SwitchPrimitives.Root's id and for both label htmlFor attributes (and keep aria-label logic unchanged), updating references in SwitchPrimitives.Root, label elements, and any related refs to consistently use effectiveId instead of switchId or id directly.
🧹 Nitpick comments (1)
packages/javascript/doodle-ui/src/components/Switch/Switch.tsx (1)
34-36: Remove commented-out code.Line 35 contains dead code from the refactor. Since the
hover:cursor-pointerbehavior is now properly handled via thedisabledvariant, this comment is no longer needed.🧹 Proposed cleanup
const LabelVariants = cva( - // 'text-base text-neutral-dark-5 transition-all ease-in-out hover:cursor-pointer dark:text-white', 'text-base text-neutral-dark-5 transition-all ease-in-out dark:text-white',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines 34 - 36, Remove the dead commented-out class from the LabelVariants cva declaration: delete the commented line containing 'hover:cursor-pointer' so LabelVariants only contains the active class string ('text-base text-neutral-dark-5 transition-all ease-in-out dark:text-white'); this cleans up leftover refactor artefacts and relies on the existing disabled variant to control cursor behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 63-84: The label htmlFor values currently always use the generated
switchId while the root id uses id ?? switchId, breaking association when a
caller passes id; compute a single effectiveId (e.g., const effectiveId = id ??
switchId) and use that for SwitchPrimitives.Root's id and for both label htmlFor
attributes (and keep aria-label logic unchanged), updating references in
SwitchPrimitives.Root, label elements, and any related refs to consistently use
effectiveId instead of switchId or id directly.
---
Nitpick comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 34-36: Remove the dead commented-out class from the LabelVariants
cva declaration: delete the commented line containing 'hover:cursor-pointer' so
LabelVariants only contains the active class string ('text-base
text-neutral-dark-5 transition-all ease-in-out dark:text-white'); this cleans up
leftover refactor artefacts and relies on the existing disabled variant to
control cursor behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b7ed358b-c356-4805-804d-8d65da5167ad
📒 Files selected for processing (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsx
KillahDillah
force-pushed
the
BED-7915-switch-refactor-pt2
branch
from
16df412 to
909c2ff
Compare
KillahDillah
commented
Apr 13, 2026
| defaultVariants: defaultSize, | ||
| } | ||
| const switchRootStyles = cva( | ||
| 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-neutral-dark-5 dark:bg-[#fff] data-[state=checked]:bg-primary dark:data-[state=checked]:bg-[#A1A0FF] dark:disabled:data-[state=checked]:bg-[#A6A6A6] disabled:cursor-not-allowed disabled:bg-[#616161] disabled:data-[state=checked]:bg-[#616161] outline-none focus:outline-none focus-visible:outline-none focus:ring-0 focus-visible:ring-0' |
Contributor
Author
There was a problem hiding this comment.
Leaving these HEX codes until we get the new color tokens merged in
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/javascript/doodle-ui/src/components/Switch/Switch.tsx (1)
57-72:⚠️ Potential issue | 🟠 MajorUse the same effective
idfor the labels and the switch root.Line 71 can render the root with a caller-provided
id, but Lines 64 and 80 still point both labels at the generated one. Any labeled switch that receivesidloses its label association, and clicking the label stops toggling the control.🔧 Suggested fix
const Switch = forwardRef<React.ElementRef<typeof SwitchPrimitives.Root>, SwitchProps>( ({ className, disabled, id, label, labelPosition = 'right', ...props }, ref) => { const ariaLabel = label || 'switch'; - const switchId = React.useId(); + const generatedId = React.useId(); + const switchId = id ?? generatedId; return ( <div className={cn(switchWrapperStyles(), label && 'py-0')}> {labelPosition === 'left' && label && ( <label className={LabelVariants({ position: 'left', disabled })} htmlFor={switchId}> @@ <SwitchPrimitives.Root ref={ref} - id={id ?? switchId} + id={switchId} aria-label={!label ? ariaLabel : undefined} className={cn(switchRootStyles(), className)} disabled={disabled} {...props}>Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines 57 - 72, The labels are using the generated switchId while SwitchPrimitives.Root can be rendered with a caller-provided id, breaking label association; update both label htmlFor attributes (the left and right label render paths) to use the same effective id passed to the root (id ?? switchId) so labels and the SwitchPrimitives.Root share the identical id, and keep the aria-label logic unchanged.
🧹 Nitpick comments (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsx (1)
100-104: Add one regression story forid+label.The current stories only exercise generated ids. Giving
Labeleda fixedidand asserting that clicking the text toggles the switch would lock down the path that currently regresses inpackages/javascript/doodle-ui/src/components/Switch/Switch.tsx.🧪 Possible story tweak
export const Labeled: Story = { args: { + id: 'switch-story-label', label: 'Label', }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const switchButton = await canvas.findByRole('switch', { name: 'Label' }); + + await userEvent.click(await canvas.findByText('Label')); + + await expect(switchButton).toHaveAttribute('aria-checked', 'true'); + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsx` around lines 100 - 104, Add a regression story that sets an explicit id on the existing Labeled story so the label is associated with the input; update the Labeled Story export in Switch.stories.tsx to include args.id (a fixed string) alongside args.label and ensure the story exercises clicking the text to toggle the Switch (so the label-for/id relationship is validated). This targets the association logic in Switch.tsx (label/input linkage) and will lock down the bug that only appears when an explicit id is present.packages/javascript/doodle-ui/src/components/Switch/Switch.tsx (1)
51-54: Make unlabeled switches opt into a real accessible name.Right now unlabeled instances fall back to
aria-label="switch". That keeps the control technically named, but it gives screen readers the same non-descriptive label for every switch and masks missing names in the unlabeled stories atpackages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxLines 55-58, 94-97, and 113-116. A small prop union that requiresaria-labeloraria-labelledbywhenlabelis absent would make this API much safer.Also applies to: 58-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines 51 - 54, The SwitchProps currently allows unlabeled switches to fall back to a generic aria-label; change the prop type to a discriminated union so that when label is provided you keep existing props (label?: string, labelPosition?: 'left'|'right'), and when label is absent require an accessible name by forcing either aria-label or aria-labelledby to be present (e.g., a union branch where label is undefined and aria-label: string | aria-labelledby: string is required). Update the Switch component type signature using this union (reference SwitchProps and React.ComponentPropsWithoutRef<typeof SwitchPrimitives.Root>) and update the unlabeled stories in Switch.stories.tsx to pass a meaningful aria-label or aria-labelledby.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 57-72: The labels are using the generated switchId while
SwitchPrimitives.Root can be rendered with a caller-provided id, breaking label
association; update both label htmlFor attributes (the left and right label
render paths) to use the same effective id passed to the root (id ?? switchId)
so labels and the SwitchPrimitives.Root share the identical id, and keep the
aria-label logic unchanged.
---
Nitpick comments:
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsx`:
- Around line 100-104: Add a regression story that sets an explicit id on the
existing Labeled story so the label is associated with the input; update the
Labeled Story export in Switch.stories.tsx to include args.id (a fixed string)
alongside args.label and ensure the story exercises clicking the text to toggle
the Switch (so the label-for/id relationship is validated). This targets the
association logic in Switch.tsx (label/input linkage) and will lock down the bug
that only appears when an explicit id is present.
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 51-54: The SwitchProps currently allows unlabeled switches to fall
back to a generic aria-label; change the prop type to a discriminated union so
that when label is provided you keep existing props (label?: string,
labelPosition?: 'left'|'right'), and when label is absent require an accessible
name by forcing either aria-label or aria-labelledby to be present (e.g., a
union branch where label is undefined and aria-label: string | aria-labelledby:
string is required). Update the Switch component type signature using this union
(reference SwitchProps and React.ComponentPropsWithoutRef<typeof
SwitchPrimitives.Root>) and update the unlabeled stories in Switch.stories.tsx
to pass a meaningful aria-label or aria-labelledby.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7d9269b4-bba5-4e74-accf-6c1556690307
📒 Files selected for processing (2)
packages/javascript/doodle-ui/src/components/Switch/Switch.stories.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.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.
Description
Describe your changes in detail
Modified colors for the focus indicator and disabled state
Motivation and Context
Resolves BED-7915
Why is this change required? What problem does it solve?
New designs with focus indicator and disabled state.
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Bug Fixes