fix(ui): Improve Floating UI usage#8328
Conversation
🦋 Changeset detectedLatest commit: 39a5e38 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.
|
|
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:
📝 WalkthroughWalkthroughAdds a Changeset and applies Floating UI and ARIA fixes across UI components: Select now generates stable trigger/listbox IDs and exposes them through state, options gain role="option" and aria-selected, the options container uses role="listbox" and accepts explicit id/label attributes, Floating UI positioning styles are applied directly, the trigger button sets aria-expanded, aria-controls and aria-haspopup="listbox", tests verify ARIA wiring, and usePopover.adjustToReferenceWidth now accepts a numeric pixel adjustment. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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/dev-cli
@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: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/ui/src/hooks/usePopover.ts`:
- Around line 63-66: The current truthy check skips width syncing when
adjustToReferenceWidth is 0; change the conditional in usePopover.ts to
explicitly allow numeric 0 by checking for typeof adjustToReferenceWidth ===
'number' or adjustToReferenceWidth === true, then compute extra as before and
set elements.floating.style.width using elements.reference.offsetWidth (or '' if
no reference); update the if condition that currently reads if
(adjustToReferenceWidth) to use this explicit check so numeric zero still
triggers width adjustment for elements.floating based on elements.reference.
🪄 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: 7318b985-c988-4a57-8e06-3dcbe2ff4574
📒 Files selected for processing (2)
packages/ui/src/elements/Select.tsxpackages/ui/src/hooks/usePopover.ts
Description
Improve Floating UI usage:
arialLabeltypo inMenuTriggerMenuListwithuseMergeRefsSelectOptionListaria-haspopuptoMenuTriggeraria-expanded,aria-haspopup,role,aria-selected) toSelectcomponents.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change