feat(blade-svelte): Switch component#3363
Conversation
Made-with: Cursor
🦋 Changeset detectedLatest commit: 6146787 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
✅ PR title follows Conventional Commits specification. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6146787:
|
| checked={isCheckedState} | ||
| disabled={isDisabled} | ||
| onchange={handleChange} | ||
| {...a11yAttrs} |
There was a problem hiding this comment.
Potential a11y conflict: duplicate aria-disabled and disabled on the input
The a11yAttrs spread includes aria-disabled (from makeAccessible({ disabled: isDisabled })), while the input also has the native disabled={isDisabled} attribute. This is technically fine for a11y -- the React version does the same -- but note that when isDisabled is false, this renders aria-disabled="false" explicitly on the element. The React snapshot shows this is expected parity, so this is consistent. Just flagging for awareness.
There was a problem hiding this comment.
Acknowledged — explicit React parity per the snapshot. No action.
| isChecked !== undefined ? isChecked : internalChecked, | ||
| ); | ||
|
|
||
| const isControlled = $derived(isChecked !== undefined); |
There was a problem hiding this comment.
isControlled should be a one-time check, not a $derived
isControlled is currently $derived(isChecked !== undefined), which means it re-evaluates every time isChecked changes. If a consumer passes isChecked={undefined} after previously passing a value (or vice versa), the component will silently switch between controlled/uncontrolled modes mid-lifecycle. This is the same pattern React warns about ('A component is changing an uncontrolled input to be controlled').
Consider computing this once on mount using untrack:
const isControlled = untrack(() => isChecked !== undefined);This matches React's behavior where the controlled/uncontrolled decision is made on initial render. In practice this is unlikely to cause bugs since Blade consumers rarely flip between modes, but it's a correctness improvement worth making.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. The mid-lifecycle uncontrolled ↔ controlled flip is the same anti-pattern React warns about at runtime; will harden in a follow-up.
| data-disabled={isDisabled || undefined} | ||
| onmousedown={handlePointerPressedIn} | ||
| onmouseup={handlePointerPressedOut} | ||
| onmouseout={handlePointerPressedOut} |
There was a problem hiding this comment.
onmouseout vs onmouseleave for pressed state reset
Using onmouseout here will fire when the cursor moves over child elements within the label (it bubbles). This could cause the pressed state to flicker if the label contained additional child elements. While the current structure is simple enough that this is unlikely to cause visible issues, onmouseleave would be more correct since it only fires when the pointer actually leaves the element boundary, not when it moves between children.
The React version uses onMouseOut as well (line 101 of Switch.tsx), so this is parity. But it might be worth switching both versions to mouseleave eventually.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. The label has no children that would re-fire mouseout today, but mouseleave is more correct. Will switch in a follow-up.
| {id} | ||
| {name} | ||
| {value} | ||
| checked={isCheckedState} |
There was a problem hiding this comment.
checked attribute will fight with Svelte's reactivity on the input
In Svelte, setting checked={isCheckedState} as a prop on a native <input type="checkbox"> only sets the initial value -- Svelte doesn't forcefully re-sync the checked DOM property on every render the way React does. Combined with the onchange handler that computes next = !isCheckedState, this works correctly for uncontrolled mode. But for controlled mode (where the parent supplies isChecked), if the parent chooses NOT to update isChecked in its onChange handler (e.g., to reject a toggle), the DOM checkbox will still visually toggle because the browser changed the checked state on click, and Svelte won't re-set it.
This is the classic Svelte controlled input problem. In Svelte 5, you may want to explicitly force the checked property in the handleChange handler:
function handleChange(event: Event): void {
if (isDisabled) return;
const next = !isCheckedState;
if (!isControlled) {
internalChecked = next;
}
onChange?.({ isChecked: next, value, event });
// Force re-sync DOM for controlled mode where parent might reject the change
if (isControlled && inputEl) {
inputEl.checked = isChecked ?? false;
}
}Or use a $effect to sync. This is only relevant when a consumer wants to 'reject' a toggle in controlled mode, which is uncommon but possible.
There was a problem hiding this comment.
Fixed in 6146787. Re-asserting inputEl.checked = isCheckedState after onChange returns handles the controlled-rejection case where the parent doesn't update isChecked and Svelte's attribute binding doesn't re-sync the DOM.
| height: var(--spacing-5); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
Empty CSS rulesets rely on CSS Modules implementation detail
These intentionally-empty rulesets (.pressed {} and .effective-checked {}) are anchors to ensure CSS Modules emits scoped class names. This is a clever workaround, but it's fragile -- some CSS minifiers or future CSS Modules implementations may strip empty rulesets. Consider adding a comment-only content or a no-op declaration (display: inherit; or similar) to make them minifier-safe. Alternatively, a build-time check or test that verifies these classes exist in the output would guard against silent breakage.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will replace the empty anchors with getSwitchTemplateClasses() references (the pattern used elsewhere in the file) in a follow-up so we don't depend on minifier behavior.
| * Payload passed to the `onChange` callback when the switch toggles. | ||
| */ | ||
| export type SwitchOnChange = (event: { isChecked: boolean; value?: string; event?: Event }) => void; | ||
|
|
There was a problem hiding this comment.
SwitchOnChange event payload: event should not be optional in the type
The type declares event?: Event (optional), but the component always passes it:
onChange?.({ isChecked: next, value, event });The event parameter in the callback will always be defined when called from the component. Making it optional in the type misleads consumers into thinking it might be undefined. Consider changing to event: Event (required) in the type definition, or at minimum documenting why it's optional (e.g., for programmatic toggling scenarios that don't originate from DOM events).
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will tighten event to required in the type in a follow-up.
rohankokane-dev
left a comment
rohankokane-dev
left a comment
There was a problem hiding this comment.
Switch Svelte 5 Migration Review
Overall Assessment
Solid migration. The component follows established blade-svelte patterns well, with proper use of $props(), $state(), $derived(), and untrack(). The CSS-in-Module + CVA architecture is well-structured and reusable. The stories are comprehensive with good coverage of controlled/uncontrolled, sizes, disabled states, and the label placement patterns from the Figma guidelines.
What Looks Good
- Accessibility fundamentals are correct:
role="switch"on the input,aria-labelviaaccessibilityLabel(required prop -- good),aria-checkedsynced to state, focus-visible ring via CSS adjacent sibling selector, track markedaria-hidden="true", SVG checkmark markedaria-hidden="true". - Svelte 5 idioms: Proper use of
$props()with destructuring and defaults,$derived()for computed values,$state()for local state,untrack()to prevent re-readingdefaultCheckedafter mount. - CSS architecture: Clean separation between blade-core styles (CSS Modules + CVA) and the Svelte component. Design token usage is consistent. Mobile breakpoint overrides at 767px match the React version's breakpoint logic.
getSwitchTemplateClasses()pattern: Smart solution to prevent tree-shaking of CSS Module classes that are only referenced in templates.- Imperative API:
export function focus()withbind:thissupport viaSwitchInstancetype -- clean parity with React'sforwardRef.
Issues Raised (inline comments)
-
[Medium] Controlled mode DOM sync (line 175): Svelte does not force-re-render checked properties like React does. If a parent rejects a toggle in controlled mode, the DOM checkbox will visually desync. Add an
$effector manual sync. -
[Low]
isControlledas$derived(line 38): Should be computed once on mount to prevent mid-lifecycle mode switching. -
[Low]
onmouseoutvsonmouseleave(line 161):mouseoutbubbles through children,mouseleavedoesn't. Parity with React, butmouseleavewould be more correct. -
[Info]
SwitchOnChangeevent optionality (types.ts line 7): Theeventfield is typed as optional but always provided. -
[Info] Empty CSS rulesets (switch.module.css line 110):
.pressed {}and.effective-checked {}anchors could be stripped by aggressive minifiers. -
[Info]
aria-disabled+disabledduplication (line 178): Matches React parity, just noting for awareness.
Missing Items per Checklist
The PR checklist shows several unchecked items:
- Component Status Page update: Not included in this PR.
- Manual testing in other browsers: Not confirmed.
- KitchenSink Story: Not present (React version has
_KitchenSink.Switch.stories.tsx). - Interaction tests: None included. Given this is an a11y-sensitive component (keyboard navigation, screen reader state), interaction tests would be valuable.
Nits (not worth inline comments)
- The
wrapperStylesderived could useundefinedas the fallback more cleanly with a ternary rather than|| undefined. - Stories file has some inline styles that could potentially use design tokens more consistently (e.g.,
gap: var(--spacing-2)vs hardcoded values). - The
capitalizehelper in stories could be imported from a shared util if one exists.
Verdict
The core implementation is correct and follows Blade patterns well. The controlled-mode DOM sync issue (point 1) is the only item I'd want addressed before merge -- the rest are improvements that can be tackled incrementally.
Address PR #3363 must-fix review item. The browser flips `inputEl.checked` synchronously on click, before `onChange` runs. In controlled mode the consumer may choose NOT to update `isChecked` in their handler (i.e. reject the toggle). When that happens, `isCheckedState` doesn't change, so Svelte's attribute binding doesn't re-sync, and the DOM checkbox is left visually toggled while the source-of-truth stays at the previous value. Re-assert the source-of-truth on `inputEl.checked` after `onChange` returns so the DOM lines up with whatever the parent (or internal state) ended up at. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Description
Migrates the React Switch component to Svelte 5.
Changes
packages/blade-svelte/src/components/Switch/packages/blade-core/src/styles/Switch/packages/blade-svelte/src/components/index.tspackages/blade-core/src/styles/index.tsAdditional Information
Artifacts (in worktree):
.cursor/artifacts/Switch/discovery-report.md.cursor/artifacts/Switch/migration-plan.md.cursor/artifacts/Switch/verification-report.md.cursor/artifacts/Switch/screenshots/See the verification report for the full validation summary.
Component Checklist
Made with Cursor