-
Notifications
You must be signed in to change notification settings - Fork 117
Conversation
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.
Just my 2c, this PR improves the menu experience immensely, Using click instead of hover is a huuuge improvement.
|
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. |
Hmmmm we could make it work for both click & hover... Would that be OK? I could push a commit later today with this tweak, should be doable with minimal changes in JS & CSS 👍 |
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. |
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 |
Added on-hover implementation and a bunch of other improvements:
|
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.
👍 This works well for me now. Thanks!
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.