fix(Tabs): ignore disabled tabs on keyboard navigation #4784
Conversation
ghost
requested review from
emyarod
force-pushed
the
2880-tabs-keyboard-navigation
branch
from
26eda89
to
1d089d5
Compare
|
Deploy preview for the-carbon-components ready! Built with commit 26eda89 https://deploy-preview-4784--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 26eda89 https://deploy-preview-4784--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 26eda89 |
|
Deploy preview for the-carbon-components ready! Built with commit f614ee6 https://deploy-preview-4784--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements failed. Built with commit f614ee6 https://app.netlify.com/sites/carbon-elements/deploys/5e166f2407e065000939f226 |
|
Deploy preview for carbon-components-react failed. Built with commit f614ee6 https://app.netlify.com/sites/carbon-components-react/deploys/5e166f24ed5a2300099f9de5 |
| }; | ||
|
|
||
| getNextIndex = (index, direction) => { | ||
| const enabledTabs = this.getEnabledTabs(); |
There was a problem hiding this comment.
Would we want to use this helper at all: https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/internal/keyboard/navigation.js#L27
There was a problem hiding this comment.
that doesn't look like a drop-in replacement for the function I have currently
There was a problem hiding this comment.
Just was speaking to figuring out the nextIndex and using a ring buffer instead of the current logic 👍
There was a problem hiding this comment.
how does it account for disabled tab indices?
There was a problem hiding this comment.
@emyarod if I understand correctly, the ring buffer would be done on a filtered down version of tabs, correct?
const tabs = [
{
title: 'Tab A',
disabled: false,
},
{
title: 'Tab B',
disabled: true,
},
{
title: 'Tab B',
disabled: false,
},
];
const enabledTabs = tabs.filter(tab => !tab.disabled);
getNextIndex(ArrowRight, 0, enabledTabs.length); // 1
getNextIndex(ArrowRight, 1, enabledTabs.length); // 0
getNextIndex(ArrowLeft, 0, enabledTabs.length); // 1Definitely get that it's not mapping exactly 1:1, but just was seeing if the pattern was worthwhile to match 👍
There was a problem hiding this comment.
the returned index from that function corresponds to the element's location in the filtered array and not the full array. but if you are just referring to the pattern then I have implemented it already, I'm just also accounting for the fact that the component is expecting the full array and not only the enabled tabs
| @@ -116,6 +117,12 @@ export default class Tabs extends React.Component { | |||
| return React.Children.map(this.props.children, tab => tab); | |||
| } | |||
|
|
|||
| getEnabledTabs = () => | |||
| React.Children.toArray(this.props.children).reduce( | |||
| (acc, tab, index) => (!tab.props.disabled ? acc.concat(index) : acc), | |||
There was a problem hiding this comment.
When we're looping through children, do we ever need to do null checks? Wasn't sure about toArray but was just remembering some issues with UI Shell where this was coming up.
There was a problem hiding this comment.
as far as I know React.Children.toArray removes nulls
emyarod
force-pushed
the
2880-tabs-keyboard-navigation
branch
from
1d089d5
to
5d62674
Compare
emyarod
force-pushed
the
2880-tabs-keyboard-navigation
branch
from
5d62674
to
7eb4cee
Compare
|
@jillianhowarth is that the case with the full story view? https://deploy-preview-4784--carbon-components-react.netlify.com/iframe.html?id=tabs--default seems the issue still persists with the normal story view but is likely storybook related |
asudoh
dismissed
joshblack’s stale review
Updating the review status given visual review is complete and we revealed that one technical review comment does not account for the specific use case for tab component.
emyarod
force-pushed
the
2880-tabs-keyboard-navigation
branch
from
07eefa2
to
6ef3515
Compare
emyarod
force-pushed
the
2880-tabs-keyboard-navigation
branch
from
6ef3515
to
2ce31ea
Compare
|
This fixes the issue with the keyboard nav on the disabled tab, but now creates another problem where it is not possible to come out from the Tabs, i.e when I hit Tab (instead of arrow keys), the focus should go to the content section. Now, it is not going to content. E.g https://codesandbox.io/s/codesandbox-86xgt I have a button under Tab3, which should take focus on pressing Tab, which was working before the fix). |
|
addressed in #5124 |
Closes #2880
Closes #1495 (our tabs are now "single tab stop")
This PR modifies the tab navigation logic to cycle through enabled tabs only rather than blindly looping through the tab list. It also resolves a performance issue when cycling through tabs
Testing / Reviewing
cycle through the tab list with the arrow keys and ensure that disabled tabs are ignored
(notice that the focus outline jump occurs at the same time as the font weight change rather than a second later)