feat(Overlay/Modal): keep position fixed #569
Merged
Conversation
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
erickcelio
changed the title
erickcelio
changed the title
erickcelio
changed the title
erickcelio
force-pushed
the
overlay-and-modal-keep-position-fixed
branch
from
2ab1377 to
a7fa6cc
Compare
blvdmitry
reviewed
Jan 14, 2026
| const { active: rendered, activate: render, deactivate: remove } = useToggle(active || false); | ||
| const { active: visible, activate: show, deactivate: hide } = useToggle(active || false); | ||
| const isDismissible = useIsDismissible({ active, contentRef, hasTrigger: false }); | ||
| const shouldBeContained = containerRef && !keepPositionFixed; |
Contributor
There was a problem hiding this comment.
wondering if we can make the API better here, a few thoughts:
- maybe we can avoid adding a prop at all in case we can detect a shadow root around the modal. Might be hard to detect since originally it doesn't render anything, but maybe we can do that while opening the modal. It also might be tricky since there can be use cases of apps using web components but still rendering modals on the top level of the document
- in case first is not an option, I would at least consider a clearer name for the prop, right now I feel how it's named after the description of the issue you're having but it might be not as clear for those who haven't used the property before. Maybe something along the lines of "displaying above the page and not the container" since that's what fixed is doing in the end
Contributor
There was a problem hiding this comment.
(actually maybe something closer to "contained" which is true by default when you pass a containerRef but can be set to false?)
Contributor
Author
There was a problem hiding this comment.
yeah, that sounds great. I think we can stick with the prop version for cases like you said, where we want to keep it contained even inside a shadow root, so we don't limit its usage
blvdmitry
reviewed
Jan 14, 2026
| blurred && s["--blurred"], | ||
| animated && s["--animated"], | ||
| containerRef && s["--contained"], | ||
| shouldBeContained && s["--contained"], |
Contributor
There was a problem hiding this comment.
Can you also run "pnpm changeset" to add a changelog item?
blvdmitry
reviewed
Jan 14, 2026
Contributor
blvdmitry
left a comment
blvdmitry
left a comment
There was a problem hiding this comment.
Looks good overall, left a few minor comments and we can merge then, thanks!
blvdmitry
approved these changes
Jan 16, 2026
Contributor
|
Thanks, will release a canary over the weekend |
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.
Summary
This will help when we need to use containerRef for some reason, but we don't want the modal to be limited to the element's size.
Related Issue
#568
Screenshots / Recordings
Notes for Reviewers