[UI Framework] Improve minor accessibility issues in ui_framework #14073
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timroes
added
Team:Platform-Design
cjcenizal
suggested changes
Sep 21, 2017
| id={idGen('collapsable')} | ||
| style={{ display: isExpanded ? 'block' : 'none' }} | ||
| > | ||
| Here is some collapsable content. |
There was a problem hiding this comment.
Typo: I think this should be "collapsible". We should probably update the generated IDs with this spelling too.
| </GuideText> | ||
|
|
||
| <GuideText> | ||
| For the sake of simplicity we haven’t labeled the element on this page |
There was a problem hiding this comment.
Is this meant to refer to the <label> example or to all of the different input examples on the page?
| You can use this button to reveal and hide content. | ||
| You can use this button to reveal and hide content. For a complete example | ||
| on how to make an collapsable panel proper accessible, read | ||
| the <a href="#/collapsebutton">CollapseButton</a> documentation. |
There was a problem hiding this comment.
Instead of using <a> directly here, let's use react-router's Link:
import {
Link,
} from 'react-router';
<Link
className="guideLink"
to="collapsebutton"
>
CollapseButton
</Link>| @@ -71,6 +71,9 @@ const KuiButton = ({ | |||
| children, | |||
| ...rest | |||
| }) => { | |||
| if (!children && !rest['aria-label'] && !rest['aria-labelledby']) { | |||
| console.error('Specify aria-label or aria-labelledby on <KuiButton/> if it does not have children.'); | |||
| } | |||
There was a problem hiding this comment.
What would you say to replacing this with a custom prop-type checker?
const accessibleIconButton = (props, propName, componentName) => {
if (props.children) {
return;
}
if (props['aria-label']) {
return;
}
if (props['aria-labelledby']) {
return;
}
throw new Error(
`${componentName} requires aria-label or aria-labelledby to be specified if it does not have children. ` +
`This is because we're assuming you're creating an icon-only button, which is screen-reader-inaccessible.`
);
};
const KuiButton = ({
isLoading,
iconPosition = DEFAULT_ICON_POSITION,
className,
buttonType,
icon,
children,
...rest
}) => {
return (
<button
className={getClassName({
className,
buttonType,
hasIcon: icon || isLoading,
})}
{...rest}
>
<ContentWithIcon
icon={icon}
iconPosition={iconPosition}
isLoading={isLoading}
>
{children}
</ContentWithIcon>
</button>
);
};
KuiButton.propTypes = {
icon: PropTypes.node,
iconPosition: PropTypes.oneOf(ICON_POSITIONS),
children: PropTypes.node,
isLoading: PropTypes.bool,
buttonType: PropTypes.oneOf(BUTTON_TYPES),
className: PropTypes.string,
'aria-label': accessibleIconButton,
};This will give us an error like this:
There was a problem hiding this comment.
That is of course the way better option. Sorry I never thought about using propTypes for checking conditions between multiple properties.
| @@ -116,6 +119,10 @@ const KuiLinkButton = ({ | |||
| } | |||
| }; | |||
|
|
|||
| if (!children && !rest['aria-label'] && !rest['aria-labelledby']) { | |||
| console.error('Specify aria-label or aria-labelledby on <KuiLinkButton/> if it does not have children.'); | |||
| } | |||
There was a problem hiding this comment.
We can reuse the same custom prop type checker here.
| @@ -6,14 +6,16 @@ import classNames from 'classnames'; | |||
| const KuiInfoButton = props => { | |||
| const iconClasses = classNames('kuiInfoButton', props.className); | |||
|
|
|||
| const ariaLabel = props['aria-label'] || 'Info'; | |||
There was a problem hiding this comment.
Instead of doing this we can create a defaultProps definition after the propTypes on line 17:
KuiInfoButton.defaultProps = {
'aria-label': 'Info',
};
|
I improved the PR with all your suggestions. |
37 tasks
|
Jenkins, test this |
1 similar comment
|
Jenkins, test this |
|
Jenkins, test this ... more and more weird errors that seem unrelated to this PR |
timroes
added a commit
that referenced
this pull request
Oct 2, 2017
* Use proper labels in bar example * Make aria-label on icon buttons mandatory * Add role=group to KuiButtonGroup * Add roles to card component * Fix alphabetical ordering in menu * Add accessibility example for KuiCollapseButton * Improve accessibility of Event examples * Add note about labeling to Form docs * Update broken snapshots * Fix icon in HeaderBar example * Add default aria-label to InfoButton if not specified * Fix wrong HTML entities * Fix icon names in Event example * Add icon labels in InfoPanel example * Improve accessibility of Micro and MenuButton examples * Add labels to StatusText example * Apply proper ARIA roles for tabs * Make ToggleButton example accessible * Fix icon names in events sandbox * Also allow aria-labelledby for icon buttons * Fix spelling of collapsible * Make statement about labels more clear * Use proper Link element for linking * Use propTypes to check for icon only buttons * Use defaultProps in KuiInfoButton
|
Backports:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes some minor accessibility issues in the UI framework. I have excluded the following components, from this PR, since they are either rather large and I would like to open separate PRs for each of them or they still need some discussion:
ColorPickerExpressionGalleryLocalNavMenuModalPagerPopoverTableAnd unrelated, but I considered it too small for it's own PR, I fixed the alphabetical order in the UI framework docs menu.
Beside some ARIA attributes and roles, this also introduces a function change:
If you use an
KuiButton(orKuiLinkButton) without any children, an error will be logged if noaria-labelandaria-labelledbyproperty was specified, to prevent the common mistake of icon only buttons.Fixes some parts of #14080