-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Lock the document from scrolling when the modal is open. #32269
Conversation
Size Change: +126 B (0%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this works in my testing.
I could use a sanity check on the JS, but I think it should be good.
It makes sense to me, but I'll caveat that by saying javascript isn't my strong suit.
Thanks for the review! I think the JS should be solid, the primary reason I'm asking is to sanity check the use of querySelector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment, otherwise LGTM!
Co-authored-by: Vicente Canales <[email protected]>
@@ -14,6 +14,10 @@ function navigationToggleModal( modal ) { | |||
triggerButton.setAttribute( 'aria-expanded', ! isHidden ); | |||
closeButton.setAttribute( 'aria-expanded', ! isHidden ); | |||
modal.classList.toggle( 'has-modal-open', ! isHidden ); | |||
|
|||
// Add a class to indicate the modal is open. | |||
const htmlElement = document.querySelector( 'html' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, can avoid using querySelector
here, the same can be achieved through const htmlElement = document.documentElement;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you! #32303
Description
Fixes #32206.
This PR applies a classname to the HTML element when the navigation burger menu is open.
This code was actually in the responsive navigation branch, here, but appears to have gotten lost along the way. The CSS to lock it from scrolling is still present.
I could use a sanity check on the JS, but I think it should be good.
How has this been tested?
Insert a navigation menu and toggle responsive. Also insert enough content so that there's a scrollbar. Then preview or publish and view the frontend, resize the viewport below 600px, then open the menu. When the menu is open, the content underneath should not scroll.
Checklist:
*.native.js
files for terms that need renaming or removal).