-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Tabs): ignore disabled tabs on keyboard navigation #4784
Changes from all commits
89d43e5
b139cee
2ce31ea
a384253
2e582b0
f97ced7
6387063
695fe48
f614ee6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import React from 'react'; | |
import classNames from 'classnames'; | ||
import { ChevronDownGlyph } from '@carbon/icons-react'; | ||
import { settings } from 'carbon-components'; | ||
import { keys, match, matches } from '../../internal/keyboard'; | ||
|
||
const { prefix } = settings; | ||
|
||
|
@@ -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), | ||
[] | ||
); | ||
|
||
getTabAt = (index, useFresh) => { | ||
return ( | ||
(!useFresh && this[`tab${index}`]) || | ||
|
@@ -139,35 +146,47 @@ export default class Tabs extends React.Component { | |
}; | ||
}; | ||
|
||
getDirection = evt => { | ||
if (match(evt, keys.ArrowLeft)) { | ||
return -1; | ||
} | ||
if (match(evt, keys.ArrowRight)) { | ||
return 1; | ||
} | ||
return 0; | ||
}; | ||
|
||
getNextIndex = (index, direction) => { | ||
const enabledTabs = this.getEnabledTabs(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that doesn't look like a drop-in replacement for the function I have currently There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just was speaking to figuring out the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does it account for disabled tab indices? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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); // 1 Definitely 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
const nextIndex = Math.max( | ||
enabledTabs.indexOf(index) + direction, | ||
-1 /* For `tab` not found in `enabledTabs` */ | ||
); | ||
const nextIndexLooped = | ||
nextIndex >= 0 && nextIndex < enabledTabs.length | ||
? nextIndex | ||
: nextIndex - Math.sign(nextIndex) * enabledTabs.length; | ||
return enabledTabs[nextIndexLooped]; | ||
}; | ||
|
||
handleTabKeyDown = onSelectionChange => { | ||
return (index, evt) => { | ||
const key = evt.key || evt.which; | ||
|
||
if (key === 'Enter' || key === 13 || key === ' ' || key === 32) { | ||
if (matches(evt, [keys.Enter, keys.Space])) { | ||
this.selectTabAt(index, onSelectionChange); | ||
this.setState({ | ||
dropdownHidden: true, | ||
}); | ||
} | ||
}; | ||
}; | ||
|
||
handleTabAnchorFocus = onSelectionChange => { | ||
return index => { | ||
const tabCount = React.Children.count(this.props.children) - 1; | ||
let tabIndex = index; | ||
if (index < 0) { | ||
tabIndex = tabCount; | ||
} else if (index > tabCount) { | ||
tabIndex = 0; | ||
} | ||
|
||
const tab = this.getTabAt(tabIndex); | ||
|
||
if (tab) { | ||
this.selectTabAt(tabIndex, onSelectionChange); | ||
if (tab.tabAnchor) { | ||
tab.tabAnchor.focus(); | ||
if (window.matchMedia('(min-width: 42rem)').matches) { | ||
evt.preventDefault(); | ||
const nextIndex = this.getNextIndex(index, this.getDirection(evt)); | ||
const tab = this.getTabAt(nextIndex); | ||
if (tab) { | ||
this.selectTabAt(nextIndex, onSelectionChange); | ||
if (tab.tabAnchor) { | ||
tab.tabAnchor.focus(); | ||
} | ||
} | ||
} | ||
}; | ||
|
@@ -222,7 +241,6 @@ export default class Tabs extends React.Component { | |
index, | ||
selected: index === this.state.selected, | ||
handleTabClick: this.handleTabClick(onSelectionChange), | ||
handleTabAnchorFocus: this.handleTabAnchorFocus(onSelectionChange), | ||
tabIndex, | ||
ref: e => { | ||
this.setTabAt(index, e); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I know
React.Children.toArray
removes nullsref https://github.com/facebook/react/blob/master/packages/react/src/__tests__/ReactChildren-test.js#L815