Modal: Improve application of body class names - #55430
Merged
Merged
Conversation
stokesman
commented
Oct 17, 2023
Comment on lines
-46
to
+48
| const level0Dismissers: MutableRefObject< | ||
| ModalProps[ 'onRequestClose' ] | undefined | ||
| >[] = []; | ||
| const ModalContext = createContext( level0Dismissers ); | ||
| const ModalContext = createContext< | ||
| MutableRefObject< ModalProps[ 'onRequestClose' ] | undefined >[] | ||
| >( [] ); |
Contributor
Author
There was a problem hiding this comment.
A level0Dismissers reference isn't needed anymore. It was added in #51602 only to maintain existing behavior. Now the effect for adding/removing body classes is nesting agnostic and simplified in that aspect.
|
Flaky tests detected in 29c962fa1bb6c34cae6bc6c06c18768f79eb9b43. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7263487451
|
mirka
approved these changes
Dec 19, 2023
mirka
left a comment
Member
There was a problem hiding this comment.
Makes sense, tests well!
Good to go with a changelog 👍
stokesman
force-pushed
the
update/modal-body-classing
branch
from
December 20, 2023 01:42
29c962f to
f4b2112
Compare
Contributor
Author
|
Thank you for reviewing Lena! |
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
* Add unit tests for body class name effects * Fix and enhance body class attribute effect * Add changelog entry
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
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.
What?
Fixes up some obscure bugginess of the
bodyOpenClassNameprop and adds a couple unit tests to specify the expected behavior.Why?
To better specify the behavior and avoid potential future issues. It doesn't seem likely this is biting anyone but it could. I found this while working on #51602.
How?
Updates the logic to account for the fact that value of the prop may differ between subsequent modals and could potentially even be changed while a modal is open.
Testing Instructions
Manually
Snippet for manual testing in the Post editor
Automatically
npm run test:unit -- components/src/modal.Screenshots or screencast
This demonstrates the two scenarios the added unit tests cover whereby the body class name is not currently added/removed as expected. The latter scenario is almost certainly never going to be encountered in the wild.
modal-body-class-fails.mp4