Multi-level Navbar Menu #123
Conversation
|
This looks good. Just a couple of extra things:
I'll review it formally after you get the tests created. |
JamesDoingStuff
force-pushed
the
jg/multilevel-menu
branch
4 times, most recently
from
88ca5c4 to
9f75851
Compare
VictoriaBeilsten-Edmands
left a comment
VictoriaBeilsten-Edmands
left a comment
There was a problem hiding this comment.
Looks good and you've handles accessibility well. Just a couple of minor points.
changelog.md
Outdated
| ### Added | ||
| - New Progress component based on Diamond Light added. | ||
| - New ProgressDelayed component so that the progress isn't shown at all when it's a small wait. | ||
| - NavMenu component added for creating dropdown menus in the NavBar |
There was a problem hiding this comment.
It's called Navbar instead of NavBar, but maybe we should change that to be more consistent with the other component names
There was a problem hiding this comment.
Yeah, good idea. Maybe it makes sense to use NavBar. And there's also AppTitleBar... They both do inherit from Bar... But... It probably shouldn't be FooterBar... BreadcrumbBar?!
Don't know. Happy for someone else to decide...!
There was a problem hiding this comment.
I don't think it needs to be a requirement for Bar-based components to have 'Bar' in the name and as nice as uniform capitalisation would be, it does seem to be the consensus online that 'navbar' is one word whereas 'app bar', 'title bar' etc. are two. I won't make any changes to it here...
|
|
||
| return ( | ||
| <> | ||
| <Button |
There was a problem hiding this comment.
Can improve accessibility a bit here with aria-haspopup, aria-expanded and aria-controls then link to the menu with id.
JamesDoingStuff
force-pushed
the
jg/multilevel-menu
branch
from
9f75851 to
dda2f86
Compare
For #52
Couple of things worth mentioning:
fireEventwas too restrictive)NavMenuLinkto work with focus I had to use ref forwarding, and to support router links without duplicating code, it now uses theNavLink, but this meant adding ref forwarding toNavLinkas well, lest the console fill up with React warnings.