feat(SideNavMenu): collapse submenu on esc keydown #5198
Conversation
ghost
requested review from
emyarod
force-pushed
the
3590-ui-shell-side-nav-esc-key-support
branch
from
3d48f84
to
0136e00
Compare
|
Deploy preview for the-carbon-components ready! Built with commit 3d48f84 https://deploy-preview-5198--the-carbon-components.netlify.com |
packages/react/src/components/UIShell/__tests__/SideNavMenu-test.js
Outdated
Show resolved
Hide resolved
|
Deploy preview for carbon-components-react failed. Built with commit 94da121 https://app.netlify.com/sites/carbon-components-react/deploys/5e31fc61278b960009a737d3 |
emyarod
force-pushed
the
3590-ui-shell-side-nav-esc-key-support
branch
from
0136e00
to
3880030
Compare
emyarod
force-pushed
the
3590-ui-shell-side-nav-esc-key-support
branch
from
3880030
to
083895c
Compare
|
Deploy preview for the-carbon-components ready! Built with commit 0136e00 https://deploy-preview-5198--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 0136e00 |
|
Deploy preview for the-carbon-components ready! Built with commit 94da121 https://deploy-preview-5198--the-carbon-components.netlify.com |
emyarod
force-pushed
the
3590-ui-shell-side-nav-esc-key-support
branch
from
083895c
to
2013dbe
Compare
|
Deploy preview for carbon-elements ready! Built with commit 083895c |
|
Deploy preview for carbon-elements ready! Built with commit 94da121 |
|
Is the up/down arrow key support already in the component btw? Just was looking at the linked issue 👀 |
asudoh
added
status: ready to merge 🎉
and removed
status: one more review 👀
status: ready for review 👀
labels
|
unless arrow key nav is a strict requirement, Tab, Shift + Tab, Space, and Enter are functional for navigation and toggling menu state. this PR will add Esc key support |
|
I'm fine either way, just was asking since the related issue listed it as a bug and wanted to see if it was being addressed in the PR. Do we know if it's a requirement? If so, we might not want the PR to close that issue. If not, then definitely makes sense to close it 👍 |
|
there were a few similar old issues that referenced lack of keyboard navigation which I closed earlier this week since it looks like we now support keyboard navigation, just not the arrow key navigation specifically. but if arrow key nav is a strict requirement we can also reopen the issue |
|
@emyarod not sure which issue you're referring too, I'm just looking at this line from #3590:
It seems like the expectation is that this follows the menu and menubar pattern, to your point I don't think we're doing that, right? cc @dakahn do you know what the true requirements are here? |
|
@joshblack I was referring to previous tickets related to the topic including the old epic created by @dakahn which mentions not strictly adhering to the menu keyboard nav guidelines |
|
Cool! Sounds good to me then 👍 Thanks! |
|
Yeah that menu pattern is controversial and introduces complications (menu being a system level designation vs the list of links or dropdowns found on the web). Here's a cool primer |
Closes #3590
This PR adds Esc key support to side nav menus so that they can be collapsed via keyboard only
Changelog
New
handleKeydownmethodTesting / Reviewing
Ensure that the Esc key functions as expected while tabbing through UI shell side nav menus