Skip to content
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.

Sign up for GitHub

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): add aria-label to overflow scroll buttons #7353

Merged
merged 4 commits into from
Jan 19, 2021
Merged

fix(Tabs): add aria-label to overflow scroll buttons #7353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d96a912
fix(Tabs): add aria-label to overflow scroll buttons
emyarod Nov 30, 2020
8df9e3d
feat(Tabs): support additional overflow button props
emyarod Nov 30, 2020
c933ff7
chore: update snapshots
emyarod Dec 10, 2020
c574d09
Merge branch 'master' into 7304-tab-overflow-button-labels
kodiakhq[bot] Jan 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5098,6 +5098,9 @@ Map {
"hidden": Object {
"type": "bool",
},
"leftOverflowButtonProps": Object {
"type": "object",
},
"light": Object {
"type": "bool",
},
Expand All @@ -5110,6 +5113,9 @@ Map {
"onSelectionChange": Object {
"type": "func",
},
"rightOverflowButtonProps": Object {
"type": "object",
},
"scrollIntoView": Object {
"type": "bool",
},
Expand Down
20 changes: 18 additions & 2 deletions packages/react/src/components/Tabs/Tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ export default class Tabs extends React.Component {
*/
hidden: PropTypes.bool,

/**
* Provide the props that describe the left overflow button
*/
leftOverflowButtonProps: PropTypes.object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we could allow folks to configure the labels without giving them a way to pass in any prop to the buttons? Maybe something like overflowStartLabel, overflowEndLabel? or scrollPrevLabel, scrollMoreLabel (just throwing out some ideas)

Exposing that this is a button and allowing folks to configure it might expose some internal implementation details, or prevent us from refactoring like if we wanted the label to be inside of the button instead of aria-label, or would want to connect it using aria-labelledby or something 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I was looking ahead and figuring that people would want to extend the button so I didn't want to make the prop too restrictive by making it label-only

the use of aria-label was suggested by @carmacleod since there is no visible label for these overflow buttons

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're anticipating folks to configure the buttons directly it might be helpful to extract them as components and allow folks to pass props directly to them (and support providing these buttons as children)

Definitely think aria-label is the right call, just was saying that this would restrict changes we make if we ever want to change how these buttons are implemented (e.g. if we wanted to use aria-labelledby for some reason).

An example of this could be the components that forward props to downshift, for example. It makes updating that package difficult as we have to manage the breaking changes in props (like name changes) if we want to update it in a minor release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't know that we need to provide that much configurability for these buttons when the logic that displays them is all internal. is this a future change that you are predicting for this component? it seems like we're solving an issue that isn't occurring given the component spec and a11y requirements


/**
* Specify whether or not to use the light component variant
*/
Expand All @@ -59,6 +64,11 @@ export default class Tabs extends React.Component {
*/
onSelectionChange: PropTypes.func,

/**
* Provide the props that describe the right overflow button
*/
rightOverflowButtonProps: PropTypes.object,

/**
* Choose whether or not to automatically scroll to newly selected tabs
* on component rerender
Expand Down Expand Up @@ -389,6 +399,8 @@ export default class Tabs extends React.Component {
scrollIntoView, // eslint-disable-line no-unused-vars
selectionMode, // eslint-disable-line no-unused-vars
tabContentClassName,
leftOverflowButtonProps,
rightOverflowButtonProps,
...other
} = this.props;

Expand Down Expand Up @@ -479,6 +491,7 @@ export default class Tabs extends React.Component {
<div {...other} className={classes.tabs} onScroll={this.handleScroll}>
<button
aria-hidden="true"
aria-label="Scroll left"
className={classes.leftOverflowButtonClasses}
onClick={(_) => this.handleOverflowNavClick(_, { direction: -1 })}
onMouseDown={(event) =>
Expand All @@ -487,7 +500,8 @@ export default class Tabs extends React.Component {
onMouseUp={this.handleOverflowNavMouseUp}
ref={this.leftOverflowNavButton}
tabIndex="-1"
type="button">
type="button"
{...leftOverflowButtonProps}>
<ChevronLeft16 />
</button>
{!leftOverflowNavButtonHidden && (
Expand All @@ -505,6 +519,7 @@ export default class Tabs extends React.Component {
)}
<button
aria-hidden="true"
aria-label="Scroll right"
className={classes.rightOverflowButtonClasses}
onClick={(_) => this.handleOverflowNavClick(_, { direction: 1 })}
onMouseDown={(event) =>
Expand All @@ -513,7 +528,8 @@ export default class Tabs extends React.Component {
onMouseUp={this.handleOverflowNavMouseUp}
ref={this.rightOverflowNavButton}
tabIndex="-1"
type="button">
type="button"
{...rightOverflowButtonProps}>
<ChevronRight16 />
</button>
</div>
Expand Down