feat(Modal): adds zIndex control and renderDismissButton#460
Open
joacotornello wants to merge 7 commits intomasterfrom
Open
feat(Modal): adds zIndex control and renderDismissButton#460joacotornello wants to merge 7 commits intomasterfrom
joacotornello wants to merge 7 commits intomasterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds z-index tokens (1000, 1100) and propagates them through tokens, theme contract, globals, and CSS properties; refactors modal CSS to expose z-index variants; extends Modal API with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
Getting startedPlease make sure you read our documentation on how to write code for components, stories and styles.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.json:
- Line 3: The allow list in .claude/settings.json is overly broad—specifically
the entries "Bash(npx eslint *)" and "Bash(git stash *)" permit wildcard
execution and state-mutating git commands; remove or tighten these entries by
eliminating "Bash(git stash *)" and replacing wildcard Bash allowances with
explicit, least-privilege commands/arguments (e.g., specific npx eslint
subcommands needed) so only the minimal exact commands are permitted; update the
"allow" array to list those exact command signatures instead of wildcard
patterns.
In `@packages/core/styles/src/packages/composite/modal/nimbus-modal.css.ts`:
- Around line 102-109: overlayScoped is a static style hardcoding zIndex[600]
and therefore doesn't follow the zIndex variant model used by overlay
(styleVariants with base/top); change overlayScoped to be a styleVariants (like
overlay) that maps the same zIndex variants (base -> zIndex[600], top ->
zIndex[1000]) and then update the Modal usage for scoped mode to pick the
correct variant (e.g., use modal.classnames.overlayScoped[zIndex] instead of
modal.classnames.overlayScoped) so scoped modals respect the zIndex prop.
- Around line 97-100: The CHANGELOG is missing a breaking-change entry
documenting that the Modal z-index values have migrated to design tokens; add a
breaking-changes bullet to the CHANGELOG.md noting that modal z-index references
(seen in overlay styleVariants using varsThemeBase.zIndex with values
600/1000/1100 via overlay and overlayPortaled) were migrated to design tokens in
v9.0.0, include the affected symbols (overlay, overlayPortaled,
varsThemeBase.zIndex) and a short migration note for consumers to update any
overrides that referenced the numeric z-index keys directly.
In `@packages/react/src/composite/Modal/src/modal.spec.tsx`:
- Around line 79-91: Extend the tests to also assert the overlay element's class
when zIndex changes: after calling makeSut (using makeSut and zIndex: "top")
query for the overlay node (e.g., via getByTestId if FloatingOverlay renders a
test id, or by querying document.body for the portal/overlay element) and add an
expectation that its className contains modal.classnames.overlay.base for the
default case and modal.classnames.overlay.top when zIndex is "top"; locate
references to makeSut, modal.classnames.container, modal.classnames.overlay,
zIndex and FloatingOverlay in the spec to implement the additional assertions.
In `@packages/react/src/composite/Modal/src/modal.stories.tsx`:
- Around line 255-292: The story modalOverSidebar mistakenly shares the same
open state for both Sidebar and Modal via useArgs and updateArgs, causing them
to open together; change the Sidebar to have independent state (e.g., set
Sidebar open always true or manage it with its own useState) while keeping the
modal controlled by useArgs/updateArgs (handleClose and open) so the modal can
be shown above an already-open sidebar; update references to Sidebar and Modal
in modalOverSidebar accordingly and ensure only Modal uses the args-provided
open/onDismiss.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcf0dbd5-7ad2-44f5-a5e4-ee119b3aa8e9
⛔ Files ignored due to path filters (2)
.yarn/versions/65f6cf1a.ymlis excluded by!**/.yarn/**packages/react/src/composite/Modal/src/modal.docs.jsonis excluded by!**/*.docs.json
📒 Files selected for processing (10)
.claude/settings.jsonpackages/core/styles/src/packages/composite/modal/nimbus-modal.css.tspackages/core/styles/src/properties/css.tspackages/core/styles/src/themes/contract.css.tspackages/core/styles/src/themes/globals.css.tspackages/core/tokens/src/zIndex/sys.jsonpackages/react/src/composite/Modal/src/Modal.tsxpackages/react/src/composite/Modal/src/modal.spec.tsxpackages/react/src/composite/Modal/src/modal.stories.tsxpackages/react/src/composite/Modal/src/modal.types.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/src/composite/Modal/src/modal.spec.tsx (1)
79-97:⚠️ Potential issue | 🟠 MajorAdd overlay z-index assertions (portal and scoped root) to complete feature coverage.
Current tests validate only
containerZIndex. The feature also applies z-index variants to overlays inModal.tsx(Line 130 and Line 147), so overlay regressions would pass unnoticed.✅ Suggested test additions
describe("WHEN zIndex selects a stacking layer", () => { @@ it("AND applies the top containerZIndex class when zIndex='top'", () => { makeSut({ children: <div>content</div>, zIndex: "top" }); const modalElement = screen.getByTestId("modal-element"); expect(modalElement.className).toContain(modal.classnames.container); expect(modalElement.className).toContain( modal.classnames.containerZIndex.top ); }); + + it("AND applies the top overlayZIndex class in portal mode", () => { + makeSut({ children: <div>content</div>, zIndex: "top" }); + const overlay = document.querySelector(`.${modal.classnames.overlay}`); + expect(overlay?.className).toContain(modal.classnames.overlayZIndex.top); + }); + + it("AND applies the top overlayScopedZIndex class in scoped mode", () => { + const root = document.createElement("div"); + document.body.appendChild(root); + makeSut({ children: <div>content</div>, zIndex: "top", root }); + const scopedOverlay = root.querySelector( + `.${modal.classnames.overlayScoped}` + ); + expect(scopedOverlay?.className).toContain( + modal.classnames.overlayScopedZIndex.top + ); + }); });As per coding guidelines,
**/*.spec.tsx: Write Jest unit tests for all components testing all component states, accessibility features, and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/composite/Modal/src/modal.spec.tsx` around lines 79 - 97, The tests currently only assert container z-index classes; add assertions that the overlay elements (both the portal overlay and the scoped-root overlay rendered by makeSut) also include the overlay base/top classes: check that each overlay element contains modal.classnames.overlay and the appropriate modal.classnames.overlayZIndex.base (for default) or modal.classnames.overlayZIndex.top (when calling makeSut with zIndex: "top"); reuse makeSut to render the modal and query the overlay nodes (by the same test render output or DOM queries used in other tests) and assert their className contains those modal.classnames values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/src/composite/Modal/src/modal.spec.tsx`:
- Around line 79-97: The tests currently only assert container z-index classes;
add assertions that the overlay elements (both the portal overlay and the
scoped-root overlay rendered by makeSut) also include the overlay base/top
classes: check that each overlay element contains modal.classnames.overlay and
the appropriate modal.classnames.overlayZIndex.base (for default) or
modal.classnames.overlayZIndex.top (when calling makeSut with zIndex: "top");
reuse makeSut to render the modal and query the overlay nodes (by the same test
render output or DOM queries used in other tests) and assert their className
contains those modal.classnames values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0151196e-211b-42de-9a12-4c826f461dd4
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by!**/.gitignore
📒 Files selected for processing (4)
.coderabbit.yamlpackages/core/styles/src/packages/composite/modal/nimbus-modal.css.tspackages/react/src/composite/Modal/src/Modal.tsxpackages/react/src/composite/Modal/src/modal.spec.tsx
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #460 +/- ##
=======================================
Coverage 96.07% 96.08%
=======================================
Files 426 426
Lines 4283 4288 +5
Branches 922 908 -14
=======================================
+ Hits 4115 4120 +5
- Misses 159 166 +7
+ Partials 9 2 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
🚀✨ Your Storybook preview is ready! Happy reviewing! 🎉 |
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish-rc.yml:
- Around line 50-55: The workflow is using actions/setup-node@v4 with an invalid
input auth-oidc and a Node.js version too old for OIDC Trusted Publishing;
remove the auth-oidc: true line from the setup step (the action ignores that
input) and bump node-version from '20' to a supported OIDC runtime (e.g., '22'
or '22.14.0'+) in the same setup block; ensure the existing explicit
NODE_AUTH_TOKEN: "" environment entry remains unchanged (required for OIDC) and
leave the yarn OIDC-related config (clearing npmAuthIdent and disabling
npmAlwaysAuth) as-is.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04574014-47a2-4114-973b-50919e96af55
📒 Files selected for processing (1)
.github/workflows/publish-rc.yml
harrytiendanube
approved these changes
Apr 24, 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
3 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.
Type
Screenshots
Changes proposed ✔️
Summary by CodeRabbit
New Features
Documentation