fix(ui): Pricing table layout shift on switch changes#8742
Conversation
🦋 Changeset detectedLatest commit: 1c07698 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR fixes a Chrome-specific scroll-jump when toggling the pricing table billing-period switch. The Switch component's root now sets position: 'relative' to provide a positioning context for absolutely-positioned internals. The visually-hidden now includes inset: 0 to constrain its bounding box. A changeset documents a patch release for Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
Break Check: no API changes detected across the tracked packages. Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/elements/Switch.tsx (1)
1-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd tests to cover the positioning changes.
No tests have been added or modified to verify the fix. While browser-specific layout issues can be challenging to test, adding interaction tests for the Switch component would provide valuable regression protection. As per coding guidelines, changes should include test coverage.
Consider adding tests that:
- Verify the Switch toggles correctly when clicked
- Verify the controlled and uncontrolled patterns work as expected
- Verify accessibility properties (role, ARIA attributes) are correctly applied
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/elements/Switch.tsx` around lines 1 - 136, Add unit/interaction tests for the Switch component to cover toggling behavior, controlled vs uncontrolled modes, and accessibility attributes: write tests that render the Switch, simulate a click and assert the visible state change (thumb position via data-checked or DOM changes) and that onChange is called; render with controlled prop isChecked and assert clicking calls onChange but does not change visual state unless prop updates; render with defaultChecked and assert internal state toggles on click; and assert role="switch", aria-label/aria-labelledby props, and disabled behavior (no state change or onChange when isDisabled=true). Target the exported Switch component and its public props (isChecked, defaultChecked, onChange, isDisabled, 'aria-label'/'aria-labelledby') in your test files.
🧹 Nitpick comments (1)
packages/ui/src/elements/Switch.tsx (1)
77-77: 💤 Low valueConsider adding a clarifying comment for the
inset: 0addition.The
inset: 0works correctly to constrain the hidden input's bounding box and fix the Chrome scroll-jump behavior. However, the interaction betweeninset: 0and the explicitwidth: '1px'andheight: '1px'fromvisuallyHidden()might be non-obvious to future maintainers.📝 Suggested clarifying comment
style={{ ...common.visuallyHidden(), + // inset: 0 constrains the bounding box to prevent Chrome scroll jump during toggle inset: 0, }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/elements/Switch.tsx` at line 77, Add a short clarifying comment next to the style entry that sets "inset: 0" (the hidden input styling in Switch.tsx) explaining that inset: 0 is required to constrain the hidden input's bounding box and prevents Chrome's scroll-jump behavior, and note that this works together with the explicit width/height from visuallyHidden() (width: '1px', height: '1px') rather than replacing them; reference the hidden input and visuallyHidden() so future maintainers understand the intent and interplay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ui/src/elements/Switch.tsx`:
- Around line 1-136: Add unit/interaction tests for the Switch component to
cover toggling behavior, controlled vs uncontrolled modes, and accessibility
attributes: write tests that render the Switch, simulate a click and assert the
visible state change (thumb position via data-checked or DOM changes) and that
onChange is called; render with controlled prop isChecked and assert clicking
calls onChange but does not change visual state unless prop updates; render with
defaultChecked and assert internal state toggles on click; and assert
role="switch", aria-label/aria-labelledby props, and disabled behavior (no state
change or onChange when isDisabled=true). Target the exported Switch component
and its public props (isChecked, defaultChecked, onChange, isDisabled,
'aria-label'/'aria-labelledby') in your test files.
---
Nitpick comments:
In `@packages/ui/src/elements/Switch.tsx`:
- Line 77: Add a short clarifying comment next to the style entry that sets
"inset: 0" (the hidden input styling in Switch.tsx) explaining that inset: 0 is
required to constrain the hidden input's bounding box and prevents Chrome's
scroll-jump behavior, and note that this works together with the explicit
width/height from visuallyHidden() (width: '1px', height: '1px') rather than
replacing them; reference the hidden input and visuallyHidden() so future
maintainers understand the intent and interplay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b0b92426-e995-43e8-b7aa-cad45d0dad13
📒 Files selected for processing (2)
.changeset/fix-switch-scroll-jump.mdpackages/ui/src/elements/Switch.tsx
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
thanks for the screen record demos 👍🏼
dstaley
left a comment
dstaley
left a comment
There was a problem hiding this comment.
approved, but remove the clerk-js entry from the changeset
Co-authored-by: Dylan Staley <88163+dstaley@users.noreply.github.com>
alexcarpenter
deleted the
alexcarpenter/bill-1705-billing-plans-page-layout-breaks-after-repeated-annual
branch
3 participants
Description
Fix Chrome-specific scroll jump when toggling the billing period switch on the pricing table.
BEFORE
before.mov
AFTER
after.mov
Fixes BILL-1705
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change