[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?
| 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?
| @@ -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?
| 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?
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