feat(headless): dialog stacking state and alertdialog role - #9427
feat(headless): dialog stacking state and alertdialog role#9427maxyinger wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: a33c2bb 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.
|
|
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:
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 Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Dialog primitive supports Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This PR updates headless dialog semantics and stacking state; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
b39c5a1 to
62387ef
Compare
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/headless/src/primitives/drawer/drawer-context.ts (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the rationale comment.
The
Omitchange is correct, but the multi-line rationale is longer than needed. Keep the explanation to one terse line that states why drawer context excludes dialog stacking state.As per coding guidelines, “Keep code comments minimal. Add comments only when critical to explain why a non-obvious change was made; never restate code behavior, and keep warranted comments to one terse line rather than a verbose multi-line block.”
Suggested change
-// -// -// The stacking pair goes with them. A drawer already counts its own nesting as -// `nestedOpenCount` / `onNested`, which is a different question from the dialog's: `isStacked` -// asks whether a DIALOG sits above, and a drawer's stacked-child styling has nothing to read it -// from. Inheriting them would oblige every drawer root to publish two values no drawer part uses. +// Drawer nesting uses `nestedOpenCount`; dialog stacking state does not belong in this context.🤖 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/headless/src/primitives/drawer/drawer-context.ts` around lines 33 - 38, Shorten the comment immediately above DrawerContextValue to one terse line explaining that drawer context excludes dialog-only stacking state. Keep the existing Omit<DialogContextValue, 'isStacked' | 'stackedChildCount' | 'store'> declaration unchanged.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.
Nitpick comments:
In `@packages/headless/src/primitives/drawer/drawer-context.ts`:
- Around line 33-38: Shorten the comment immediately above DrawerContextValue to
one terse line explaining that drawer context excludes dialog-only stacking
state. Keep the existing Omit<DialogContextValue, 'isStacked' |
'stackedChildCount' | 'store'> declaration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d2e396e-5086-4c87-9364-d1a7a3719544
📒 Files selected for processing (1)
packages/headless/src/primitives/drawer/drawer-context.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)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
Publish "still covering" rather than the raw `open` flag from `DialogNestingContext`, so a stacked child keeps `data-stacked` for the length of the parent's exit instead of painting a second scrim over the parent's fading one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fb78cab to
3e567d0
Compare
Publish "still covering" rather than the raw `open` flag from `DialogNestingContext`, so a stacked child keeps `data-stacked` for the length of the parent's exit instead of painting a second scrim over the parent's fading one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3e567d0 to
677b258
Compare
Adds the two signals the Mosaic stacking styles need, and the role an AlertDialog preset needs. `data-stacked` / `data-stack-base` describe dialog-on-dialog specifically, in both directions of the relationship. `data-nested` could not: it reports any floating ancestor, so a dialog opened from a menu item reads as nested while sitting on the bare page. Under the incoming rule that a stacked dialog paints no backdrop, styling off `data-nested` would leave that dialog with no scrim at all. The child registers with its parent while OPEN rather than while mounted, so a dialog beneath comes forward with its child's exit transition rather than after it. The count is of direct children only, which is enough for the single recede step that exists.
`DrawerContextValue` inherits from `DialogContextValue`, so adding `isStacked` and `stackedChildCount` there made every drawer root fail to satisfy its own context — `tsc --noEmit` was red for the whole package, and for swingset, which typechecks headless from source. They belong in the same `Omit` as `store`. A drawer already tracks its nesting as `nestedOpenCount`, and `isStacked` asks a question about DIALOGS that a drawer has nothing to answer with.
Publish "still covering" rather than the raw `open` flag from `DialogNestingContext`, so a stacked child keeps `data-stacked` for the length of the parent's exit instead of painting a second scrim over the parent's fading one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
677b258 to
a33c2bb
Compare
2 participants
Description
roleto accept'dialog'or'alertdialog'and defaults to'dialog'data-stackeddata-stack-basepanel → prompt → alertstackdata-nesteddata-nestedstill exists with existing behaviorChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change