Make NavMenu Keyboard Navigable #5772
Conversation
| @@ -16,7 +16,7 @@ import Tooltip, { TooltipProps } from '@material-ui/core/Tooltip'; | |||
| import { makeStyles } from '@material-ui/core/styles'; | |||
|
|
|||
| const NavLinkRef = forwardRef<HTMLAnchorElement, NavLinkProps>((props, ref) => ( | |||
| <NavLink innerRef={ref} {...props} /> | |||
| <NavLink innerRef={ref} {...props} tabIndex={0} /> | |||
There was a problem hiding this comment.
it looks like the {...props} spread operator contains a tabIndex={-1} which would override the one I explicitly pass through if I were to pass it before the spreaded props.
I'm not sure what the most appropriate way to handle this. The way I've done it is most straightforward, but it currently prevents users from enabling/disabling tabbing on their own accord. Is this configuration that we'd want to give users?
There was a problem hiding this comment.
This is because material UI MenuItem set it to -1 when not specified. You should add this tabIndex prop to the MenuItem component below in the same file
There was a problem hiding this comment.
Good shout, it feels much more appropriate being set in MenuItem, I've pushed the changes.
|
Nice! Indeed, an integration test would be welcome. |
I've tested this manually.
I haven't added any unit tests, but I'll be happy to add an integration test that ensures that the menu item is reachable via the tab button.
tab.index.mov