-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: indentation level of modals #3515
Open
kark
wants to merge
2
commits into
main
Choose a base branch
from
SHIELD-1088-fix-indentation-level-of-modeals
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains 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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: f289bba The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
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 |
kark
added
🚧 Status: WIP
Work in progress
👨⚕️ Type: Tests
and removed
👨⚕️ Type: Tests
labels
May 7, 2024
kark
force-pushed
the
SHIELD-1088-fix-indentation-level-of-modeals
branch
from
May 8, 2024 08:58
8a947de
to
1589f38
Compare
kark
changed the title
fix: indendation level of modals
fix: indentation level of modals
May 8, 2024
refactor: inside
kark
force-pushed
the
SHIELD-1088-fix-indentation-level-of-modeals
branch
from
May 8, 2024 09:08
1589f38
to
1209eaa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR addresses an issue encountered with the usage of the
useModalState
hook for managing modal state within our documentation. Currently, following the steps as outlined in the documentation snippets doesn't allow applying correct indentation levels for modals stacking layers due to a lack of DOM mutation events that can be picked up by the Mutation Observer used within the portals container.The problem is that following the steps from our snippets e.g.
doesn't really work (i.e.
Modal
component from thereact-modal
library technically triggers a DOM mutation, by applying a grandchild element of the portals container, but the Mutation Observer doesn't react to it).Until now the conditional rendering was the solution to make it work instead:
With this PR, whenever the
useModalState
hook is used to manage modal state andisModalState
changes, the necessary child-level DOM mutation is triggered. This ensures that the Mutation Observer can properly compute the correct indentation level for the modal's stacking layer.Alternatives considered
Technically there is a
subtree
option that can be passed to the Mutation Observerobserve
method call, but it didn't work either.I suspect it might be related to https://stackoverflow.com/a/61315048 but I felt that creating a custom Tree Walker is an overkill (but maybe that would be your preferred solution)