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

feat(PieChart): improve a11y #1619

Merged
merged 2 commits into from
Aug 16, 2018
Merged

feat(PieChart): improve a11y #1619

merged 2 commits into from
Aug 16, 2018

Conversation

jsomsanith-tlnd
Copy link
Contributor

@jsomsanith-tlnd jsomsanith-tlnd commented Aug 15, 2018 •
edited

What is the problem this PR is trying to solve?

  • On loading state, no busy indication or text-alternative
  • Svg chart has no way to accept text-alternative

What is the chosen solution to this problem?

  • Add aria-busy and loading aria-label
  • Spread extra props into svg instead of span container

Please check if the PR fulfills these requirements

  • The PR commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features) And non reg done before need review
  • Docs have been added / updated (for bug fixes / features)
  • Related design / discussions / pages (not in jira), if any, are all linked or available in the PR

[x] This PR introduces a breaking change
Extra props provided to the component are now spread on the svg element, not the span container.

Example:

import PieChart from '@talend/react-components/lib/PieChart';

<PieChart aria-label='My text alternative' />

Before

<span arial-label='My text alternative'>
    <svg></svg>
    <span></span>
</span>

After

<span>
    <svg arial-label='My text alternative'></svg>
    <span></span>
</span>

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

jdescottes-talend
jdescottes-talend reviewed Aug 16, 2018
View reviewed changes
packages/components/src/PieChart/PieChartIcon.component.js
@@ -301,12 +305,13 @@ export function PieChartIconComponent({
// to keep only the event listener from the TooltipTrigger.
const omitI18N = omit(rest, ['i18n', 'tReady']);
return (
<span className={classnames(theme['tc-pie-chart-icon'], 'tc-pie-icon-chart')} {...omitI18N}>
<span className={classnames(theme['tc-pie-chart-icon'], 'tc-pie-icon-chart')}>
<svg
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a breaking change, and the commit don't mentioned it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at the PR description and the dedicated wiki page

jdescottes-talend
jdescottes-talend suggested changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@jdescottes-talend jdescottes-talend left a comment

Choose a reason for hiding this comment

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

It needs a breaking change mention

@jsomsanith-tlnd
Copy link
Contributor Author

jsomsanith-tlnd commented Aug 16, 2018 •
edited

@jdescottes-talend that's already done :

  • in PR description
  • in wiki --> in next release note

@jsomsanith-tlnd jsomsanith-tlnd merged commit 2a177a3 into master Aug 16, 2018
@jsomsanith-tlnd jsomsanith-tlnd deleted the jsomsanith/feat/a11y_pie-chart branch August 16, 2018 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romainseb romainseb romainseb approved these changes

@jdescottes-talend jdescottes-talend jdescottes-talend approved these changes

Assignees
No one assigned
Labels
easy review need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jsomsanith-tlnd @build-travis-ci @romainseb @jdescottes-talend