feat(ui): split the Mosaic popover from its surface - #9295
Closed
maxyinger wants to merge 7 commits into
Closed
Conversation
Reusable StyleX-styled popover built on the @clerk/headless popover primitive: a flexible popup card with content + footer slots, enter/exit transition driven off self data-attributes, and swingset docs.
# Conflicts: # packages/swingset/src/lib/registry.ts
# Conflicts: # packages/ui/src/mosaic/styles/index.ts
The popover now owns only what it means to float: trigger wiring, ARIA, focus management, positioning, stacking, viewport clamps, and the enter/exit transition. It paints no surface of its own, so the content inside it — usually a Card — supplies the background, border, radius, shadow and padding, and only one element ever draws a border. Popover.Content and Popover.Footer are removed; they duplicated Card.Content and Card.Footer. Popover.Arrow is removed as unused. The popup gains a `size` variant (sm/md/lg), reflected as `data-size`. `md` reproduces the width the legacy PopoverCard uses (theme.sizes.$94) so popovers migrating onto Mosaic keep their footprint. The positioner is always role="dialog" but only gains aria-labelledby once a Popover.Title mounts, so a popover with neither a Title nor a label was an unnamed dialog. PopoverProps now accepts aria-label and aria-labelledby, and a dev-only check warns when the rendered dialog has neither. Both are spread conditionally: an explicit `undefined` would delete the Title's aria-labelledby through the primitive's mergeProps. The positioner and popup rules move to a shared floating.styles.ts so other floating surfaces can reuse them, and use the motion theme tokens — `--cl-duration-base`, `linear` for the fade and `--cl-ease-default` for the scale. Under prefers-reduced-motion only `transform` leaves the transition; the fade still runs, since the movement is the concern.
A styled menu over @clerk/headless/menu, inheriting its roving focus, typeahead, nested submenus, and role="menu" / role="menuitem" wiring. Deliberately not built on Popover. A menu's list semantics — arrow-key navigation, typeahead, the submenu tree — have no equivalent in a role="dialog" popover, and Popover.Positioner throws outside a Popover.Root, so the styled parts cannot be reused across the two. What they do share is the floating box, now in floating.styles.ts: stacking, viewport clamps, and the enter/exit transition. The chrome-free rule does not carry over. A menu's popup is its surface, so it owns background, border, radius, shadow and inset padding, sizes to its content with a floor rather than a fixed width scale, and scrolls itself. Rows reuse the Item geometry so a menu row and a list row are the same shape. Only the background lives here, because a row highlights on data-active — the roving-focus position — as well as on hover, and StyleX merges by property, so one declaration has to cover every state. Rows take the coarse-pointer target floor while staying compact for a mouse. Menu.SubTrigger opens a nested submenu. It exists because a nested Menu.Trigger already registers itself as a menuitem in the parent list, so wrapping a Menu.Item in one would register a second entry and desync arrow-key navigation from what is on screen. Menu.Item falls back to rendering its `label` when given no children; the primitive keeps them separate because `label` drives typeahead, but they are almost always the same string.
Adds a Menu page and rewrites the Popover page for the surface split. The popover examples now compose a Card inside the popover, making that the canonical shape so the docs supply the consistency the primitive no longer enforces on its own. Adds placement and alignment demos, an accessible-name section, and drops the removed Content, Footer, and Arrow parts from the parts table. The placement demos reserve vertical room around the trigger: without it the flip middleware bounces a `top` popover back below the trigger inside the short preview frame, so the demo would contradict its label. The Menu page documents the list semantics the popover does not have, and explains when to reach for each. Both story files move onto the remapped Button API. The trigger render props are typed as Omit<React.HTMLAttributes<HTMLElement>, 'color'> — the native `color` is a string and collides with Button's narrowed variant, which is the same omission props.ts makes for the same reason.
🦋 Changeset detectedLatest commit: 634c3da The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
Contributor
📝 WalkthroughWalkthroughAdds Mosaic Menu and Popover components built on headless primitives, with StyleX styling, compound APIs, accessibility behavior, and interaction tests. Adds shared floating-surface transitions and data-state styling guidance. Exports the components through Mosaic styles, adds Swingset stories and MDX documentation, and registers both components in the documentation viewer and story registry. Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 5
🧹 Nitpick comments (1)
packages/ui/src/mosaic/components/popover/index.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRemove the new component-local barrel files.
packages/ui/src/mosaic/components/popover/index.ts#L1-L2: expose Popover from the designated package entry or direct module path instead of thisindex.ts.packages/ui/src/mosaic/components/menu/index.ts#L1-L2: expose Menu from the designated package entry or direct module path instead of thisindex.ts.As per coding guidelines, “Avoid barrel files (index.ts re-exports) as they can cause circular dependencies.”
🤖 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/mosaic/components/popover/index.ts` around lines 1 - 2, Remove the component-local barrel re-exports from packages/ui/src/mosaic/components/popover/index.ts lines 1-2 and packages/ui/src/mosaic/components/menu/index.ts lines 1-2. Update consumers to import Popover and Menu from the designated package entry points or their direct modules instead.Source: Coding guidelines
🤖 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 @.changeset/mosaic-popover.md:
- Around line 1-2: Add a non-empty Changesets release entry in the frontmatter
of mosaic-popover.md for the packages/ui package, using the appropriate version
bump and a concise summary describing the new public Menu and Popover APIs.
In `@packages/swingset/src/stories/menu.component.stories.tsx`:
- Around line 18-25: Update the menuTrigger story in
packages/swingset/src/stories/menu.component.stories.tsx (lines 18-25) and the
corresponding story in
packages/swingset/src/stories/popover.component.stories.tsx (lines 21-23) to
replace React.HTMLAttributes<HTMLElement> with HTMLAttributes<HTMLElement>,
adding a type-only import for HTMLAttributes from react in each file.
In `@packages/swingset/src/stories/popover.component.mdx`:
- Around line 72-80: Each runnable Popover snippet needs an accessible name. In
packages/swingset/src/stories/popover.component.mdx lines 72-80, 90-97, and
125-133, update the corresponding Popover components by adding an aria-label or
a Popover.Title, ensuring every dialog has a name without changing the examples’
existing behavior.
In `@packages/ui/src/mosaic/components/popover/popover.tsx`:
- Around line 48-49: Update the accessible-name check in the popover dialog
validation to inspect the trimmed values of aria-label and aria-labelledby
rather than only attribute presence. Suppress the warning only when either
attribute contains non-whitespace text, while preserving the existing return
behavior for valid names.
- Around line 133-145: Add an explicit React.JSX.Element return type to the
exported Popover component in
packages/ui/src/mosaic/components/popover/popover.tsx (lines 133-145) and the
exported Menu component in packages/ui/src/mosaic/components/menu/menu.tsx
(lines 134-151), preserving their existing implementations.
---
Nitpick comments:
In `@packages/ui/src/mosaic/components/popover/index.ts`:
- Around line 1-2: Remove the component-local barrel re-exports from
packages/ui/src/mosaic/components/popover/index.ts lines 1-2 and
packages/ui/src/mosaic/components/menu/index.ts lines 1-2. Update consumers to
import Popover and Menu from the designated package entry points or their direct
modules instead.
🪄 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 Plus
Run ID: 7ba435c8-7797-47f9-9d07-0b374e0882ac
📒 Files selected for processing (18)
.changeset/mosaic-popover.md.claude/skills/mosaic/references/stylex.mdpackages/swingset/src/components/DocsViewer.tsxpackages/swingset/src/lib/registry.tspackages/swingset/src/stories/menu.component.mdxpackages/swingset/src/stories/menu.component.stories.tsxpackages/swingset/src/stories/popover.component.mdxpackages/swingset/src/stories/popover.component.stories.tsxpackages/ui/src/mosaic/components/floating.styles.tspackages/ui/src/mosaic/components/menu/index.tspackages/ui/src/mosaic/components/menu/menu.styles.tspackages/ui/src/mosaic/components/menu/menu.test.tsxpackages/ui/src/mosaic/components/menu/menu.tsxpackages/ui/src/mosaic/components/popover/index.tspackages/ui/src/mosaic/components/popover/popover.styles.tspackages/ui/src/mosaic/components/popover/popover.test.tsxpackages/ui/src/mosaic/components/popover/popover.tsxpackages/ui/src/mosaic/styles/index.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
Comment on lines
+1
to
+2
| --- | ||
| --- |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a release entry for the new public UI APIs.
An empty changeset will omit the exported Menu and Popover additions from package versioning and release notes. Add the appropriate packages/ui bump and summary.
As per coding guidelines, “Use Changesets for version management and changelogs.” Based on learnings, empty changesets are only acceptable for documentation-only or non-published changes.
🤖 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 @.changeset/mosaic-popover.md around lines 1 - 2, Add a non-empty Changesets
release entry in the frontmatter of mosaic-popover.md for the packages/ui
package, using the appropriate version bump and a concise summary describing the
new public Menu and Popover APIs.
Sources: Coding guidelines, Learnings
Comment on lines
+18
to
+25
| const menuTrigger = (props: Omit<React.HTMLAttributes<HTMLElement>, 'color'>) => ( | ||
| <Button | ||
| variant='outline' | ||
| {...props} | ||
| > | ||
| Actions | ||
| </Button> | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -H -t f 'tsconfig*.json' . -x sh -c '
echo "--- $1"
rg -n "\"allowUmdGlobalAccess\"|\"jsx\"|\"types\"" "$1"
' sh {}
rg -n --glob '*.tsx' 'React\.HTMLAttributes' packages/swingset/src/storiesRepository: clerk/javascript
Length of output: 4125
🏁 Script executed:
#!/bin/bash
set -eu
echo "--- swingset tsconfig"
cat -n packages/swingset/tsconfig.json
echo
echo "--- root tsconfig relevant settings"
python3 - <<'PY'
import json
p='tsconfig.json'; data=json.load(open(p))
print(json.dumps(data.get('compilerOptions', {}), indent=2, sort_keys=True))
PY
echo
echo "--- imports in story files"
sed -n '1,40p' packages/swingset/src/stories/menu.component.stories.tsx
sed -n '1,80p' packages/swingset/src/stories/popover.component.stories.tsx
sed -n '1,80p' packages/swingset/src/stories/dialog.component.stories.tsxRepository: clerk/javascript
Length of output: 6449
Import HTMLAttributes directly.
These module-scoped stories are not importing React, so React.HTMLAttributes still relies on a UMD global unless the project is explicitly configured to allow it. Use a type-only import instead:
packages/swingset/src/stories/menu.component.stories.tsxpackages/swingset/src/stories/popover.component.stories.tsxpackages/swingset/src/stories/dialog.component.stories.tsx
Replace React.HTMLAttributes<HTMLElement> with HTMLAttributes<HTMLElement> and import it via import type { HTMLAttributes } from 'react';.
📍 Affects 2 files
packages/swingset/src/stories/menu.component.stories.tsx#L18-L25(this comment)packages/swingset/src/stories/popover.component.stories.tsx#L21-L23
🤖 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/swingset/src/stories/menu.component.stories.tsx` around lines 18 -
25, Update the menuTrigger story in
packages/swingset/src/stories/menu.component.stories.tsx (lines 18-25) and the
corresponding story in
packages/swingset/src/stories/popover.component.stories.tsx (lines 21-23) to
replace React.HTMLAttributes<HTMLElement> with HTMLAttributes<HTMLElement>,
adding a type-only import for HTMLAttributes from react in each file.
Source: Coding guidelines
Comment on lines
+72
to
+80
| <Popover | ||
| open={open} | ||
| onOpenChange={setOpen} | ||
| trigger={props => <Button {...props}>Open</Button>} | ||
| > | ||
| <Card> | ||
| <Card.Content>Flexible inner content.</Card.Content> | ||
| </Card> | ||
| </Popover>; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Name every Popover in runnable snippets.
These examples create role="dialog" popovers without an accessible name, contradicting the page’s own requirement and producing development warnings when copied.
packages/swingset/src/stories/popover.component.mdx#L72-L80: addaria-labelor aPopover.Titleto the controlled example.packages/swingset/src/stories/popover.component.mdx#L90-L97: addaria-labelor aPopover.Titleto the size example.packages/swingset/src/stories/popover.component.mdx#L125-L133: addaria-labelor aPopover.Titleto the alignment example.
📍 Affects 1 file
packages/swingset/src/stories/popover.component.mdx#L72-L80(this comment)packages/swingset/src/stories/popover.component.mdx#L90-L97packages/swingset/src/stories/popover.component.mdx#L125-L133
🤖 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/swingset/src/stories/popover.component.mdx` around lines 72 - 80,
Each runnable Popover snippet needs an accessible name. In
packages/swingset/src/stories/popover.component.mdx lines 72-80, 90-97, and
125-133, update the corresponding Popover components by adding an aria-label or
a Popover.Title, ensuring every dialog has a name without changing the examples’
existing behavior.
Comment on lines
+48
to
+49
| if (node.hasAttribute('aria-label') || node.hasAttribute('aria-labelledby')) { | ||
| return; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not treat empty ARIA attributes as an accessible name.
hasAttribute() suppresses the warning for aria-label="" or aria-labelledby="", leaving an unnamed dialog without development feedback. Check trimmed attribute values instead.
Proposed fix
- if (node.hasAttribute('aria-label') || node.hasAttribute('aria-labelledby')) {
+ const ariaLabel = node.getAttribute('aria-label')?.trim();
+ const ariaLabelledby = node.getAttribute('aria-labelledby')?.trim();
+ if (ariaLabel || ariaLabelledby) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (node.hasAttribute('aria-label') || node.hasAttribute('aria-labelledby')) { | |
| return; | |
| const ariaLabel = node.getAttribute('aria-label')?.trim(); | |
| const ariaLabelledby = node.getAttribute('aria-labelledby')?.trim(); | |
| if (ariaLabel || ariaLabelledby) { | |
| return; |
🤖 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/mosaic/components/popover/popover.tsx` around lines 48 - 49,
Update the accessible-name check in the popover dialog validation to inspect the
trimmed values of aria-label and aria-labelledby rather than only attribute
presence. Suppress the warning only when either attribute contains
non-whitespace text, while preserving the existing return behavior for valid
names.
Comment on lines
+133
to
+145
| export function Popover({ | ||
| trigger, | ||
| children, | ||
| open, | ||
| defaultOpen, | ||
| onOpenChange, | ||
| modal, | ||
| placement, | ||
| sideOffset, | ||
| size, | ||
| 'aria-label': ariaLabel, | ||
| 'aria-labelledby': ariaLabelledby, | ||
| }: PopoverProps) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nUP 'export function (Popover|Menu)\b[\s\S]*?\)\s*\{' \
packages/ui/src/mosaic/components/popover/popover.tsx \
packages/ui/src/mosaic/components/menu/menu.tsxRepository: clerk/javascript
Length of output: 1288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target function lines =="
sed -n '125,170p' packages/ui/src/mosaic/components/popover/popover.tsx
sed -n '125,160p' packages/ui/src/mosaic/components/menu/menu.tsx
echo
echo "== exports / barrel references =="
sed -n '1,60p' packages/ui/src/mosaic/components/popover/index.ts 2>/dev/null || true
sed -n '1,60p' packages/ui/src/mosaic/components/menu/index.ts 2>/dev/null || true
rg -n "from ['\"].*/components/(popover|menu)/index['\"]|components/popover/index|components/menu/index|export .*Popover|export .*Menu" packages/ui/src/mosaic -g '*.{ts,tsx}' | head -80 || true
echo
echo "== lint/ts config return typing hints =="
rg -n "explicitFunctionReturnTypes|explicitModuleBoundaryTypes|React\.JSX\.Element|declare (function|const) \\w+: React\\.JSX\\.Element" . --glob '*.json' --glob '*.ts' --glob '*.tsx' --glob '*.js' | head -120 || trueRepository: clerk/javascript
Length of output: 5211
Declare explicit return types for the public Mosaic components.
Popover and Menu are exported and re-exported by packages/ui/src/mosaic, so annotate both with React.JSX.Element per the TS component guideline.
📍 Affects 2 files
packages/ui/src/mosaic/components/popover/popover.tsx#L133-L145(this comment)packages/ui/src/mosaic/components/menu/menu.tsx#L134-L151
🤖 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/mosaic/components/popover/popover.tsx` around lines 133 -
145, Add an explicit React.JSX.Element return type to the exported Popover
component in packages/ui/src/mosaic/components/popover/popover.tsx (lines
133-145) and the exported Menu component in
packages/ui/src/mosaic/components/menu/menu.tsx (lines 134-151), preserving
their existing implementations.
Sources: Coding guidelines, Learnings
This was referenced Jul 30, 2026
alexcarpenter
added a commit
that referenced
this pull request
Jul 31, 2026
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
2 participants
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
Continues
carp/mosaic-popover— that branch's two commits (the original MosaicPopoverand the StyleX data-attribute docs) are included in this diff.Mosaic
Popovernow owns only what it means to float — trigger, ARIA, positioning, stacking, viewport clamps, transition. It paints no surface: the content inside it (usually aCard) supplies background, border, radius and shadow, so only one element draws a border.Popover.Content/Popover.Footer, which duplicatedCard.Content/Card.Footer, and the unusedPopover.Arrow.size(sm/md/lg, reflected asdata-size);mdmatches the legacyPopoverCardwidth so migrating popovers keep their footprint.aria-label/aria-labelledby. The positioner is alwaysrole="dialog"but had no accessible name unless aPopover.Titlewas rendered; a dev-only check now warns when neither is present.Menuon the@clerk/headlessmenu primitive —role="menu", roving focus, typeahead, and submenus viaMenu.SubTrigger. Unlike the popover, a menu's popup is its surface.floating.styles.ts. Motion uses--cl-duration-base,linearfor the fade and--cl-ease-defaultfor the scale; reduced motion drops onlytransform.Known limitation: the popup caps its height at
min(80dvh, 36rem)but clips rather than scrolls. The scroll region belongs toCard.Content, which gains it inaustin/mosaic-card-component.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change