[Menubar] Base implementation of refactored NavBar #3279
Conversation
… in a header container, removed navigation role from UserMenu
…resentation to or
There was a problem hiding this comment.
Thanks so much for your work on this here and sorry that it took a while to look over this!
Reading through the article you provided was really helpful, and I think a lot of the proposed changes here make sense! I added in a few notes and questions to specific files, but overall feel that this looks pretty good so far!
I think the only noticeable issue that I ran into is that the UserMenu dropdown on the right-hand side is not appearing for me when I toggle it. I was also wondering if you planned to apply a similar structure to the UserMenu as well?
I was also interested in the design choice to move the LanguageMenu to the left! (I'll add a screenshot here just in case for other folks who might be viewing this.) Could you provide a bit more context behind this, since I don't think I saw it mentioned in the original issue!
I feel like I've put quite a bit of questions here so this is the last 😂 I was wondering did you plan for this PR to be merged first before the other one you submitted earlier, that handles adding keyboard/arrow-key navigation?
Thanks so much again for your work on this PR! I really appreciate how intentionally and thoroughly your process has been laid out!
| @@ -0,0 +1,100 @@ | |||
| import classNames from 'classnames'; | |||
There was a problem hiding this comment.
I really, really love the changes here and the way this file is organized!
I know it's already within the tagged issue, but do you think it might be helpful to reference the article you've used as a comment here? Just came to mind since we have a few areas throughout the app that make these types of annotations!
There was a problem hiding this comment.
Also found this article that touches on implementing focus traps for dropdown menus. Was wondering if you feel that might be applicable here, or if it might be unnecessary?
There was a problem hiding this comment.
I really, really love the changes here and the way this file is organized!
I know it's already within the tagged issue, but do you think it might be helpful to reference the article you've used as a comment here? Just came to mind since we have a few areas throughout the app that make these types of annotations!
absolutely -- will add it as a comment so that it's clearer as a reference.
Also found this article that touches on implementing focus traps for dropdown menus. Was wondering if you feel that might be applicable here, or if it might be unnecessary?
thanks for this! i feel that focus trapping is something i associate more with modals and dialogs, where users can't tab out of the element unless a certain action is performed. i think we also want to be able to tab to the next element from the menubar without completely restricting focus in the way that we would with modals / dialogs / overlays. i'll see if i can find a menubar implementation in the wild that uses focus trapping, but here it might not be unnecessary.
that being said, we will definitely need a level of focus management to handle tabbing into the component as well as focusing various submenus / menuitems -- in the examples i've found, this has been done with a roving tabindex.
| return ( | ||
| <li className={classNames('nav__item', isOpen && 'nav__item--open')}> | ||
| <MenubarTrigger id={id} title={title} /> | ||
| <MenubarList id={id}>{children}</MenubarList> |
There was a problem hiding this comment.
Would it maybe make sense to conditionally render the MenubarList when isOpen is true as well?
There was a problem hiding this comment.
Would it maybe make sense to conditionally render the
MenubarListwhenisOpenistrueas well?
great point. the wai aria menubar example doesn't conditionally render its list component, but I've seen component libraries handle this by rendering the component within a portal. definitely something that can be built in!
| @@ -4,13 +4,13 @@ import { sortBy } from 'lodash'; | |||
| import { Link } from 'react-router-dom'; | |||
| import PropTypes from 'prop-types'; | |||
| import { useTranslation } from 'react-i18next'; | |||
| import NavDropdownMenu from '../../../../components/Nav/NavDropdownMenu'; | |||
| import NavMenuItem from '../../../../components/Nav/NavMenuItem'; | |||
| import MenubarMenu from '../../../../components/Menubar/MenubarMenu'; | |||
There was a problem hiding this comment.
I feel that the overall change from Nav to MenuBar makes sense!
This is a minor detail and probably more subjective, but I was wondering if maybe the name "MenuBarMenu" in particular might be a little confusing?
There was a problem hiding this comment.
yeah I think I feel the same! I can somewhat differentiate between a "Menubar" and "Menu" component but seeing it named like that may be confusing at first glance. using something like "Submenu" might make more sense
| aria-hidden="true" | ||
| /> | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
Would aria-live also need to be added here, or is the current implementation sufficient?
There was a problem hiding this comment.
based on what i've read about aria-live, it's most used to announce dynamic updates to page content even if that area isn't focused on. i think a menubar with proper focus management and keyboard controls will allow screen readers to naturally announce any new changes or contexts, so there might not be a use for it here.
we could consider using aria-live in conjunction with the language menu, since selecting an option there updates the entire page. I can do some tests with a screen reader and see how that goes!
no worries at all, appreciate you giving this some time!
oops did not catch this! i guess it might have to do with moving the dropdown outside of the i made this change because the options within a
yes of course, thanks for pointing this out! i made this change for similar reasons as moving the
yes to this! i found that in the other PR, I was making these same changes anyway so it felt appropriate to open a separate PR that focused on refactoring the code. when this PR is finished, i think it makes sense to merge first. successive PRs will implement keyboard / cursor behavior and tests. that being said i might just close that initial one and open a new PR for keyboard / mouse behaviors, because there may be duplicate commits and i'm not sure how to cleanly maintain the branch given the changes in this one. if that feels unnecessary let me know, just afraid of a messy commit history / creating more work for you and other maintainers!
thanks to you as well! all of this feedback is very thoughtful and clarifying |
Progresses #3250
Base refactor of the NavBar component. The current component is semantically closer to a menubar than navigation element, so these changes are made with that pattern in mind. Keyboard and pointer behaviors to follow along with tests.
Changes:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123