feat(tabs): support home/end keyboard navigation #3258
Conversation
emyarod
force-pushed
the
1495-tabs-keyboard-a11y
branch
from
bf39a48
to
23ea20c
Compare
|
Deploy preview for the-carbon-components ready! Built with commit bf39a48 https://deploy-preview-3258--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit bf39a48 https://deploy-preview-3258--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit bf39a48 |
|
Deploy preview for the-carbon-components ready! Built with commit bfdc52a https://deploy-preview-3258--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit bfdc52a https://deploy-preview-3258--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit bfdc52a |
|
@joshblack yeah if there is significant overlap, although I need some clarification about the original ticket WRT the arrow key vs tab navigation. or were you just referring to home/end key navigation? |
|
@emyarod we can definitely revisit it post this PR, just was noticing some similarity in these different widgets that we might be able to consolidate. |
|
@joshblack agreed, I think there is some shared logic we can combine and once the navigation pattern is fleshed out for these components it will be clearer |
emyarod
force-pushed
the
1495-tabs-keyboard-a11y
branch
from
23ea20c
to
ea9d72a
Compare
emyarod
force-pushed
the
1495-tabs-keyboard-a11y
branch
from
4313217
to
3f484cc
Compare
| } else if (index > tabCount) { | ||
| tabIndex = 0; | ||
| handleTabAnchorFocus = onSelectionChange => (evt, { index }) => { | ||
| const newIndex = (() => { |
There was a problem hiding this comment.
Does this need to be wrapped in an IIFE?
There was a problem hiding this comment.
not really, but we immediately execute the function and assign the return value to newIndex this way
There was a problem hiding this comment.
@emyarod right, I'm just curious why we need to do this versus hoisting it to the parent scope?
There was a problem hiding this comment.
not sure I fully understand your question, but it references index so it needs to be within the scope of handleTabAnchorFocus
|
|
||
| describe('state: selected', () => { | ||
| it('updates selected state when pressing arrow keys', () => { | ||
| firstTab.simulate('keydown', { which: rightKey }); | ||
| firstTab.simulate('keydown', { ...ArrowRight }); |
There was a problem hiding this comment.
| firstTab.simulate('keydown', { ...ArrowRight }); | |
| firstTab.simulate('keydown', ArrowRight); |
If you want! Although I'm sure you had a good reason for the spread 👍
There was a problem hiding this comment.
just to get all of the properties in the simulated event and not just which
There was a problem hiding this comment.
@emyarod not sure I understand, it seems like you're calling out going from which to ArrorRight, I was just suggesting there was no need to spread the object as it works inline as well
emyarod
force-pushed
the
1495-tabs-keyboard-a11y
branch
from
3f484cc
to
e786bdf
Compare
emyarod
force-pushed
the
1495-tabs-keyboard-a11y
branch
from
e786bdf
to
2190ad4
Compare
This reverts commit cb25e3e.
Closes #1495
This PR adds
HomeandEndkey support for the tabs component and addresses an issue where disabled tabs can still be focused via keyboardChangelog
New
Changed
Tabs.getTabsnow includesindexinformation on each tabTabs.handleKeydownrefactored to use the internal keyboard event handler toolsTabs.handleTabAnchorFocusto ignore disabled tabsTesting / Reviewing
Ensure there are no regressions with the tabs component keyboard navigation
Ensure that the home and end keydown event handlers behave as expected
Ensure that disabled tabs cannot be focused