fix(ui): Rework OrganizationSwitcher and UserButton popover animations#9113
fix(ui): Rework OrganizationSwitcher and UserButton popover animations#9113maxyinger wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 789c9ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
📝 WalkthroughWalkthroughThe Popover component adds an ChangesPopover exit animation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Popover
participant useTransitionStyles
participant usePrefersReducedMotion
Consumer->>Popover: render with animateExit=true
Popover->>usePrefersReducedMotion: check reduced motion preference
Popover->>Popover: compute isMotionSafe (animations enabled and not reduced)
alt animateExit enabled and isMotionSafe
Popover->>useTransitionStyles: request isMounted, styles
Popover->>Popover: derive shouldRender from isMounted
Popover->>Popover: clone child element merging transition styles
end
Popover-->>Consumer: render content gated by shouldRender
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@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: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/elements/Popover.tsx (2)
60-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
200duplicates the theme token instead of referencing it.The comment claims
close"matches the theme's$slow(200ms) transition duration" but hardcodes the literal instead of referencing that token.Drawer.tsxuses a sharedtransitionDurationValues.drawerconstant for the same purpose, avoiding drift if the theme value changes.♻️ Proposed fix
- duration: { open: 0, close: animateExit && isMotionSafe ? 200 : 0 }, + duration: { open: 0, close: animateExit && isMotionSafe ? transitionDurationValues.slow : 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/Popover.tsx` around lines 60 - 72, The Popover transition duration is hardcoded to 200 instead of using the shared theme-derived value, which can drift from the actual token. Update the `useTransitionStyles` config in `Popover` to reference the same shared transition-duration constant/pattern used by `Drawer.tsx` (for the close duration) so the timing stays aligned with the theme token.
76-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnsafe generic assertion on
isValidElement.
React.isValidElement<{ style?: React.CSSProperties }>(children)only confirmschildrenis a valid React element — it doesn't verify the element actually accepts astyleprop. If a non-DOM component (e.g. a component without a passthroughstyle) is ever passed aschildrenwithanimateExit, the cloned prop would be silently dropped/ignored by that component. Currently safe in practice since consumers only pass DOM-like elements, but worth a comment noting the assumption.🤖 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/Popover.tsx` around lines 76 - 81, The generic on React.isValidElement in Popover.tsx is only assuming the child accepts a style prop, not verifying it. Update the content/cloneElement branch in Popover to add a clear comment near the React.isValidElement/React.cloneElement logic explaining that animateExit is only intended for DOM-like children that forward style, and that non-DOM components may ignore the injected styles.
🤖 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/elements/Popover.tsx`:
- Around line 105-112: The non-portal branch in Popover is dropping the
outsideElementsInert prop, so it never reaches FloatingFocusManager when portal
is false. Update the Popover rendering path that uses FloatingFocusManager to
pass outsideElementsInert through there as well, matching the portal-based
branch so the prop works consistently in both cases.
---
Nitpick comments:
In `@packages/ui/src/elements/Popover.tsx`:
- Around line 60-72: The Popover transition duration is hardcoded to 200 instead
of using the shared theme-derived value, which can drift from the actual token.
Update the `useTransitionStyles` config in `Popover` to reference the same
shared transition-duration constant/pattern used by `Drawer.tsx` (for the close
duration) so the timing stays aligned with the theme token.
- Around line 76-81: The generic on React.isValidElement in Popover.tsx is only
assuming the child accepts a style prop, not verifying it. Update the
content/cloneElement branch in Popover to add a clear comment near the
React.isValidElement/React.cloneElement logic explaining that animateExit is
only intended for DOM-like children that forward style, and that non-DOM
components may ignore the injected styles.
🪄 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), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9439f1f7-4c7a-4777-8c69-277d23109fda
📒 Files selected for processing (6)
.changeset/popover-enter-exit-animations.mdpackages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsxpackages/ui/src/components/OrganizationSwitcher/index.tsxpackages/ui/src/components/UserButton/UserButtonPopover.tsxpackages/ui/src/components/UserButton/index.tsxpackages/ui/src/elements/Popover.tsx
| {shouldRender && ( | ||
| <FloatingFocusManager | ||
| context={context} | ||
| initialFocus={initialFocus} | ||
| order={order} | ||
| modal={modal} | ||
| > | ||
| <>{children}</> | ||
| <>{content}</> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/ui/src/elements/Popover.tsx' 'packages/ui/src/**' | sed -n '1,120p'
echo '--- search outsideElementsInert ---'
rg -n "outsideElementsInert|FloatingFocusManager" packages/ui/src -S
echo '--- Popover.tsx around relevant lines ---'
nl -ba packages/ui/src/elements/Popover.tsx | sed -n '1,220p'Repository: clerk/javascript
Length of output: 8979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/ui/src/elements/Popover.tsx')
print('exists:', p.exists())
if p.exists():
text = p.read_text()
for needle in ['outsideElementsInert', 'FloatingFocusManager', 'portal']:
print(f'-- {needle} --')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: clerk/javascript
Length of output: 862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' packages/ui/src/elements/Popover.tsx | cat -nRepository: clerk/javascript
Length of output: 4850
Forward outsideElementsInert in the non-portal branch
portal={false} currently drops outsideElementsInert, so the prop has no effect there. Pass it through to FloatingFocusManager in the non-portal return as well.
🤖 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/Popover.tsx` around lines 105 - 112, The non-portal
branch in Popover is dropping the outsideElementsInert prop, so it never reaches
FloatingFocusManager when portal is false. Update the Popover rendering path
that uses FloatingFocusManager to pass outsideElementsInert through there as
well, matching the portal-based branch so the prop works consistently in both
cases.
1 participant
Description
Fast is a feature. this improves snappyness of our org switcher and user button popover anim. since it's a user initiated action and the affected item is right by the mouse, there's no need for an entrance anim. I think all the anim is doing in this case is making the interaction feel slower since there's no need to guide the eye. this also adds an exit animation which feels nice. follows osx interaction patterns and feels right, but don't see it executed on the web much. lets be one of the first to do it right.
Avatar flash bug was fixed in this PR. figured this was best to not lump in with that cus it might be contentious, but it's good stuff.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes