Skip to content

Global Header/Navigation: Switch to observer for menu open & close actions #682

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

Merged
merged 1 commit into from
Jul 1, 2025

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Apr 14, 2025

Fixes #678. The global header nav adds a class when opened, has-global-modal-open, but this was only being removed when the nav was closed by button click. This changes the way that class is removed by watching the has-modal-open class, which the core block adds, so that the custom class is always removed when the nav modal closes.

I then used the same observer to fix #681, by simply scrolling to the top of the page when a nav is opened. I did try to fix the alignment via CSS, but it's fixed to the viewport position. Switching to absolute + some other tweaks worked, but then the bottom was cut off.

Screen Shot 2025-04-14 at 18 42 58

So I'm on the fence about whether this is too hacky — it does make the screen jump, but then everything is positioned perfectly… Here's an example:

nav-scroll.mp4

To test

Global header fix:

  1. Use a viewport < 890px
  2. Open the global header menu
  3. Hit escape
  4. It should close the menu, and behave like before the menu was open
    • The local navigation bar should still be visible
    • The global header should not be sticky

Detached menu fix:

  1. Use a viewport < 600px
  2. Scroll down slightly
  3. Open the global header or local navigation dropdown
  4. It should scroll back to the top of the page
  5. The open menu's close button should be correctly aligned with the previous open button
  6. None of the page itself should be visible

This issue ^ appears on both the global header menus and local nav menus, so you can test both.

@Jiwoon-Kim
Copy link

I was thinking, would it be possible to implement a scroll-responsive sticky header for this feature? the header could hide when the user scrolls down and reappear when scrolling up. This approach could help improve the user experience by minimizing the header's space when not needed, while still providing easy access when the user needs it.

Also, how about implementing the menu in an off-canvas style? This could provide a clean and modern design, saving space while offering easy access to the navigation items. The menu could slide in from the side when triggered (via a button or swipe), and close when clicked outside or when an item is selected.

Would love to hear thoughts on this approach—let me know if it's feasible and if you think it could be added!

@ryelle
Copy link
Contributor Author

ryelle commented Apr 17, 2025

We've gone through a few iterations on the sticky menu idea to land where we are today. You can see some of the historical discussion here #465 (comment).

Also, how about implementing the menu in an off-canvas style?

The header and menu we have now is done mostly with core blocks (with some adjustments for design), so unless this can be done with core blocks, I don't think it's likely to change. Smaller fixes for the current header would be more likely to be implemented. However, if you'd like to propose something, you could start a discussion.

@ryelle ryelle moved this to 👀 In review (PRs only) in WordPress.org May 5, 2025
@ryelle ryelle moved this from 👀 In review (PRs only) to 🚀 Ready for deploy (PRs only) in WordPress.org Jun 3, 2025
@dd32
Copy link
Member

dd32 commented Jun 27, 2025

So I'm on the fence about whether this is too hacky — it does make the screen jump, but then everything is positioned perfectly… Here's an example:

This does feels too hacky.. and the screen jump isn't amazing IMHO.. however, the positioning and visual is more important than the minor screen jump and the code seems like it's relatively simple, and I'm not really familiar with MutationObserver..

Just noting that I've seen this, am going to double-check it by testing, and will merge+deploy.

@dd32 dd32 merged commit d9bb484 into trunk Jul 1, 2025
2 checks passed
@dd32 dd32 deleted the fix/nav-menu-positioning branch July 1, 2025 01:59
@github-project-automation github-project-automation bot moved this from 🚀 Ready for deploy (PRs only) to ✅ Done in WordPress.org Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Global submenu incorrectly positioned with scroll. Bug in Mobile Layout – Navigation Menu Behavior*
3 participants