fix: hide modal close button when not dismissible #1641
Conversation
The close button in the modal header now only renders when the modal is dismissible. This prevents confusion where users see a close button but the modal cannot be dismissed by clicking outside or pressing Escape. Changes: - Add dismissible prop to ModalContext - Conditionally render close button in ModalHeader based on dismissible - Add tests to verify close button visibility behavior
🦋 Changeset detectedLatest commit: e8d03b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@ebbbang is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/components/Modal/Modal.tsx (1)
85-85: Consider the breaking change implications of thefalsedefault.With
dismissibledefaulting tofalse, existing code that doesn't explicitly pass this prop will no longer show the close button (previously it always showed). While this fixes the inconsistency described in the PR and creates more logical behavior, it could surprise users upgrading the library.Consider:
- Adding a migration note in the changelog
- Documenting the
dismissibleprop clearly in component documentation- Potentially adding a console warning in development mode if the prop is undefined (though this might be too noisy)
Would you like me to help draft migration guidance or documentation for this change?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ui/src/components/Modal/Modal.test.tsx(2 hunks)packages/ui/src/components/Modal/Modal.tsx(1 hunks)packages/ui/src/components/Modal/ModalContext.tsx(1 hunks)packages/ui/src/components/Modal/ModalHeader.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Modal/ModalHeader.tsx (1)
apps/web/examples/modal/modal.dismissible.tsx (1)
dismissible(78-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🧰 Build
🔇 Additional comments (7)
packages/ui/src/components/Modal/ModalContext.tsx (1)
9-9: LGTM! Clean context extension.The addition of the optional
dismissibleproperty to the context interface is straightforward and follows the existing pattern (similar to thepopupproperty).packages/ui/src/components/Modal/ModalHeader.tsx (2)
34-34: LGTM! Correct context consumption.Properly extracts the
dismissibleflag from the modal context for use in conditional rendering.
68-72: LGTM! Correct conditional rendering.The close button now only renders when the modal is dismissible, which ensures consistent behavior—non-dismissible modals won't show a close button that wouldn't work anyway. The existing
aria-label="Close"maintains accessibility when the button is rendered.packages/ui/src/components/Modal/Modal.test.tsx (3)
29-41: LGTM! Good test coverage for non-dismissible case.This test correctly verifies that the close button is not rendered when
dismissibleis not set (defaults tofalse). UsingqueryByLabelTextis the right choice since it returnsnullwhen the element is not found.
43-55: LGTM! Good test coverage for dismissible case.This test correctly verifies that the close button is rendered when
dismissible={true}is explicitly set.
113-113: LGTM! Correct test update.This test needs
dismissible={true}to ensure the close button is rendered so it can be clicked. Good catch updating this existing test.packages/ui/src/components/Modal/Modal.tsx (1)
120-120: LGTM! Correctly threads prop through context.The
dismissibleprop is now properly passed through the context provider, making it available toModalHeaderfor conditional rendering of the close button. This creates consistent behavior wheredismissiblecontrols both outside-click dismissal (line 104) and close button visibility.
SutuSebastian
left a comment
SutuSebastian
left a comment
There was a problem hiding this comment.
LGTM!
One thing left:
- run
bun run changeset - select
flowbite-reactpackage - bump as
patch
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Done |
2 participants
Summary
Fixes the modal close button to only render when the modal is dismissible. Previously, the X button in the modal header would always display, even when
dismissible={false}, which created a confusing UX where the button appeared but had no effect.Changes
dismissibleprop toModalContextinterfaceModalcomponent to passdismissiblethrough contextModalHeaderto conditionally render the close button based ondismissiblepropdismissibleprop where neededBehavior
dismissible={true}Test Plan
dismissibleprop shows no close buttondismissible={true}shows close buttonScreenshots
N/A - behavioral change only
These follow the project's conventions from the contributing guide, including the commitizen-style prefix and the Claude Code attribution footer.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.