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

Jump to bottom

fix(tooltip): add aria-labelledby #3011

Closed
wants to merge 13 commits into from
Closed

fix(tooltip): add aria-labelledby #3011

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c7a32d9
fix(tooltip): add aria-labelledby
dakahn Jun 11, 2019
12f685f
Merge branch 'master' into 2736-tooltip
joshblack Jun 11, 2019
eedde50
fix(tooltip): add aria-label support
dakahn Jun 12, 2019
6d02e5c
Merge branch '2736-tooltip' of github.com:dakahn/carbon into 2736-too…
dakahn Jun 12, 2019
598cefe
Merge branch 'master' of github.com:carbon-design-system/carbon into …
dakahn Jun 13, 2019
654db70
fix(tooltip): bust out nested ternary into function
dakahn Jun 14, 2019
a790c4d
fix(tooltip-definition): remove getInstanceOf generated ID
dakahn Jun 14, 2019
231bf7e
Merge branch 'master' into 2736-tooltip
joshblack Jun 20, 2019
e1a5371
Merge branch 'master' into 2736-tooltip
joshblack Jun 25, 2019
d5dc43a
Merge branch 'master' into 2736-tooltip
dakahn Jun 26, 2019
53af591
fix(tooltip): fix undefined for iconDescription
dakahn Jun 26, 2019
9484f22
fix(tooltip): update snappies
dakahn Jun 26, 2019
e71dcaa
Merge branch 'master' into 2736-tooltip
emyarod Jun 28, 2019
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
30 changes: 19 additions & 11 deletions packages/react/src/components/Tooltip/Tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ class Tooltip extends Component {
iconName: PropTypes.string,

...isRequiredOneOf({
/**
* A general purpose aria-label to be read by screen readers
*/
ariaLabel: PropTypes.string,

/**
* The content to put into the trigger UI, except the (default) tooltip icon.
*/
Expand Down Expand Up @@ -306,6 +311,18 @@ class Tooltip extends Component {
}
};

getAccessibleLabel = (ariaLabel, triggerText, triggerId) => {
// if the user provides and aria-label go with that
if (ariaLabel) {
return { 'aria-label': ariaLabel };
// if the user provides triggerText -- set aria-labelledby to that element's ID
} else if (triggerText) {
return { 'aria-labelledby': triggerId };
}
// if the user provides neither of those set aria-label to icon-description
return { 'aria-label': iconDescription };
Copy link
Member

Choose a reason for hiding this comment

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

how are you getting iconDescription here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, getting back to this now. :D That's just a prop. Are you thinking it should be required if we don't have either ariaLabel or triggerText?

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is it looks like it's undefined. you would need to get the value from this.props.iconDescription first

};

render() {
const {
triggerId = (this.triggerId =
Expand All @@ -321,6 +338,7 @@ class Tooltip extends Component {
children,
className,
triggerClassName,
ariaLabel,
direction,
triggerText,
showIcon,
Expand Down Expand Up @@ -363,17 +381,7 @@ class Tooltip extends Component {
onBlur: this.handleMouse,
'aria-haspopup': 'true',
'aria-expanded': open,
// if the user provides property `triggerText`,
// then the button should use aria-describedby to point to its id,
// if the user doesn't provide property `triggerText`,
// then an aria-label will be provided via the `iconDescription` property.
...(triggerText
? {
'aria-describedby': triggerId,
}
: {
'aria-label': iconDescription,
}),
...this.getAccessibleLabel(ariaLabel, triggerText, triggerId),
};

return (
Expand Down
8 changes: 2 additions & 6 deletions packages/react/src/components/TooltipDefinition/TooltipDefinition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ import cx from 'classnames';
import React from 'react';
import PropTypes from 'prop-types';
import { settings } from 'carbon-components';
import setupGetInstanceId from '../../tools/setupGetInstanceId';

const { prefix } = settings;
const getInstanceId = setupGetInstanceId();
const TooltipDefinition = ({
id,
className,
Expand All @@ -22,7 +20,6 @@ const TooltipDefinition = ({
tooltipText,
...rest
}) => {
const tooltipId = id || `definition-tooltip-${getInstanceId()}`;
const tooltipClassName = cx(`${prefix}--tooltip--definition`, className);
const tooltipTriggerClasses = cx(
`${prefix}--tooltip__trigger`,
Expand All @@ -36,7 +33,7 @@ const TooltipDefinition = ({
<div {...rest} className={tooltipClassName}>
<button
className={tooltipTriggerClasses}
aria-describedby={tooltipId}
aria-describedby={id}
aria-label={tooltipText}>
{children}
</button>
Expand All @@ -63,8 +60,7 @@ TooltipDefinition.propTypes = {
align: PropTypes.oneOf(['start', 'center', 'end']),

/**
* Optionally specify a custom id for the tooltip. If one is not provided, we
* generate a unique id for you.
* Optionally specify a custom id for the tooltip.
*/
id: PropTypes.string,

Expand Down