Skip to content
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

Global header: Update navigation menu items #458

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Sep 26, 2023

Fixes #316, fixes #360, fixes #440, fixes #417. This updates the menu structure to match the layout in this comment: #360 (comment)

Additionally, this changes the menu behavior to "open on click", rather than hover, which avoids the duplicate menu items. Changing this required some more CSS and JS changes than expected, so it would be good to get another 👀 on that just in case.

There should be no major visual changes, the main visible difference is that now the focus goes around both the title and dropdown icon.

Screenshot 2023-09-26 at 6 25 27 PM
Screenshot
Showcase, no menus open new-header-showcase
Showcase, small screens new-header-mobile
Blue header, with a menu open new-header-blue

To test

  • Try on a sandbox or with one of the redesign projects
  • The menu items should match the issue comment
  • Items with submenus should have chevron icons
  • Hovering over items will highlight them but not open submenus
  • Clicking an item with a submenu should open the menu, not change page navigation
  • Non-submenu items should behave as expected too

Try on Rosetta sites if you can sandbox.

Warning

This should not be merged until Showcase is ready to be moved to the top-level, once the new theme launches.

Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Working nicely for me on Developer.

I noticed that when tabbing with my keyboard at mobile size I can focus the submenu items, eg. Extend, but can't act upon them, maybe an accessibility concern. I know it's not usual user behaviour but maybe mobile assistive technologies will focus things in a similar manner.

@ndiego
Copy link
Member

ndiego commented Sep 27, 2023

From the screenshots, this is looking great! I will need to figure out how to set up a testing environment 😅

@ndiego
Copy link
Member

ndiego commented Sep 27, 2023

I noticed that when tabbing with my keyboard at mobile size I can focus the submenu items, eg. Extend, but can't act upon them, maybe an accessibility concern. I know it's not usual user behaviour but maybe mobile assistive technologies will focus things in a similar manner.

In testing with the Navigation block, if you don't have a URL set for nav links with a submenu, tabbing does not focus on the nav links (in this case Extend, etc.). So I think if you remove the # and set no URL at all (here for example), the tabbing should work as expected.

Copy link
Contributor

@renintw renintw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great enhancement! 👍

A few notes:

  1. Tickets that were mentioned have been tested and appear to have no issues.
  2. The comment in Navigation: Rephrase a few items, reconsider sorting #360 doesn't seem to have reached a conclusion yet. The ticket might need to remain open or the discussion could be moved to a new ticket after this PR gets merged.
  3. For this commit, I've noticed the ellipsis on es.wp.org, and it works as expected.
  4. Regarding Nick's comment, whether I set the URL to an empty string or delete the entire line, both tabbing and screen readers still focus on the item. Maybe I've mistaken the context or suggestion. It also doesn't seem to work by removing the URL here in the header.php directly.

@ndiego
Copy link
Member

ndiego commented Oct 2, 2023

Regarding Nick's #458 (comment), whether I set the URL to an empty string or delete the entire line, both tabbing and screen readers still focus on the item. Maybe I've mistaken the context or suggestion. It also doesn't seem to work by removing the URL here in the header.php directly.

Looks like I was wrong. Sorry for the confusion.

@ryelle
Copy link
Contributor Author

ryelle commented Oct 2, 2023

I noticed that when tabbing with my keyboard at mobile size I can focus the submenu items, eg. Extend, but can't act upon them, maybe an accessibility concern.

Yeah, it looks like this is a gutenbug— the mobile menu is intentionally expanded, but the semantics on the submenu toggle buttons make it seem like it's still possible to expand/collapse them. aria-expanded is initially false, and toggled on click, despite the menu always being expanded.

The comment in #360 doesn't seem to have reached a conclusion yet. The ticket might need to remain open or the discussion could be moved to a new ticket after this PR gets merged.

I think the resolution there was that Hosting should be a top-level menu item, so the discussion of a local menu on Download is not necessary.

@ndiego
Copy link
Member

ndiego commented Oct 2, 2023

I think the resolution there was that Hosting should be a top-level menu item, so the discussion of a local menu on Download is not necessary.

Yeah, adding Hosting back as a top-level menu item makes that comment mute.

@renintw renintw merged commit c375eb6 into trunk Oct 19, 2023
2 checks passed
@renintw renintw deleted the update/header-menu-items branch October 19, 2023 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants