Skip to content
This repository has been archived by the owner on Dec 2, 2020. It is now read-only.

Update primary navigation #263

Merged
merged 10 commits into from
Oct 5, 2020
Merged

Update primary navigation #263

merged 10 commits into from
Oct 5, 2020

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Oct 4, 2020

Removes aria-haspopup -this is only intended for application menus, not navigation.
Moves the aria-expanded to the menu toggle button.
Fixes a problem with the aria-controls, that was pointing to an incorrect class, adding a new ID to the first ul.

Adds a toggle button for sub menus.
Clicking the button toggles the sub menus. Sub menus no longer open on hover or focus.

When the sub menu is closed, the plus icon shows, and when open, the minus icon shows.

The button is not visible in the mobile menu.

Removes aria-haspopup
Moves the aria-expanded to the button.
Fixes the aria-controls, adding a new ID to the ul.
Adds a toggle button for sub menus.
@carolinan carolinan marked this pull request as ready for review October 5, 2020 08:04
@aristath
Copy link
Member

aristath commented Oct 5, 2020

Just my 2c, this PR improves the menu experience immensely, Using click instead of hover is a huuuge improvement.

  • Hover does not, under any circumstances imply intent
  • It's semantically more correct
  • My erratic finger movements when browsing don't trigger menus expanding
  • when I want to expand them it works perfectly
  • I don't have to worry about keeping the cursor in the narrow submenus otherwise I'll need to start over
  • Perhaps and most importantly, tabbing is no longer frustrating! I can cycle through the main menu items without having to go through all the submenus in my 100-items test site ❤️

@kjellr
Copy link
Collaborator

kjellr commented Oct 5, 2020

I definitely appreciate the improvements to keyboard navigation here. From using this with a mouse though, my first impression is that this feels a little utilitarian — it works, but it was a little surprising at first and its' not something I particularly enjoy interacting with. Some of that could likely be tidied up with some slight adjustments to animation/etc. But I think the biggest issue I experience personally is that it adds extra work to view the submenu.

While I agree that hover doesn't always imply intent, the hover-to-expand-submenus pattern is so prevalent that I think people expect it. Users are also very used to hovering over the entire parent item to expand the children. In this PR however, the submenu only expands up on iteration with the plus icon — and not just a hover there, but a full click. If I were looking around for a specific sub menu item for instance, I think it'd be frustrating to manually click to open and close a few parent menu items to explore them.

@aristath
Copy link
Member

aristath commented Oct 5, 2020

Hmmmm we could make it work for both click & hover... Would that be OK?
This way mouse users would get the "expected" on-hover behavior, and keyboard users would get a better experience.

I could push a commit later today with this tweak, should be doable with minimal changes in JS & CSS 👍

@kjellr
Copy link
Collaborator

kjellr commented Oct 5, 2020

That might work — as long as hover on the parent item (not just the plus) would be possible. Otherwise I imagine many folks would hover over the parent item, see that nothing happens, and assume it's broken.

@aristath
Copy link
Member

aristath commented Oct 5, 2020

Yeah, we'll make it work, I'll push some tweaks soon.

As a small parenthesis, historically this whole "expand menus on hover" thing started with a bad implementation and a false premise that users should be able to get anywhere in under 3 clicks (the infamous and many times proved to be detrimental to user-experience 3-click rule). By now unfortunately this pattern has been so over-used that people expect it 😅

@aristath
Copy link
Member

aristath commented Oct 5, 2020

Added on-hover implementation and a bunch of other improvements:

  • Close submenus when we tab-away from them
  • Close submenus when clicking outside them
  • Other minor tweaks

Copy link
Collaborator

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

👍 This works well for me now. Thanks!

@aristath aristath merged commit 2a420e0 into trunk Oct 5, 2020
@aristath aristath deleted the primary-nav branch October 5, 2020 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants