feat(a11y): allow user to pass custom description for screen readers#1111
feat(a11y): allow user to pass custom description for screen readers#1111rshen91 merged 12 commits intoelastic:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 72.22% 72.68% +0.46%
==========================================
Files 387 404 +17
Lines 12007 12353 +346
Branches 2623 2677 +54
==========================================
+ Hits 8672 8979 +307
- Misses 3310 3341 +31
- Partials 25 33 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
api/charts.api.md
Outdated
| debug: boolean; | ||
| // @alpha | ||
| debugState?: boolean; | ||
| disableGeneratedSeriesTypes: boolean; |
There was a problem hiding this comment.
Just reading this prop name, I'm not sure what it means... What do you think about something like disableGeneratedDescription?
There was a problem hiding this comment.
I also really like the way you named both properties in the issue example:
description and useDefaultSummary
they are generic and simple.
the word custom doesn't add much to that property meaning
There was a problem hiding this comment.
Looks like you missed this file for renaming these variables
| annotationSpecs: AnnotationSpec[]; | ||
| panelGeoms: PanelGeoms; | ||
| seriesTypes: Set<SeriesType>; | ||
| customDescription: string | undefined; |
There was a problem hiding this comment.
I think it's effectively equivalent but I think defining this as an optional prop communicates the intent a little better.
| customDescription: string | undefined; | |
| customDescription?: string; |
| }} | ||
| // eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role | ||
| role="presentation" | ||
| aria-describedby="get_custom_description" |
There was a problem hiding this comment.
You'll want to replace this with a randomly generated id – if a user renders two charts, each with a unique description, you'll end up getting only the first id read off for both charts.
You'll also want to conditionally add the attribute. An empty description will sometimes still get read out (something like "description empty") but probably just want to ignore it.
| aria-describedby="get_custom_description" | |
| {...(customDescription ? {'aria-describedby': randomId} : {})} |
There was a problem hiding this comment.
you can use the chartId, that is random and unique per chart, + some postfix
There was a problem hiding this comment.
Ok I am open to feedback about 3b13cc9 thank you!
| annotationSpecs: AnnotationSpec[]; | ||
| panelGeoms: PanelGeoms; | ||
| seriesTypes: Set<SeriesType>; | ||
| customDescription: string | undefined; |
There was a problem hiding this comment.
We should also accept something along the lines of a descriptionId. If the description is already rendered somewhere else on the page, we want users to be able to just point to it.
There was a problem hiding this comment.
Ok let me know if I'm following - 3b13cc9 thank you!
| }} | ||
| // eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role | ||
| role="presentation" | ||
| aria-describedby="get_custom_description" |
There was a problem hiding this comment.
you can use the chartId, that is random and unique per chart, + some postfix
api/charts.api.md
Outdated
| debug: boolean; | ||
| // @alpha | ||
| debugState?: boolean; | ||
| disableGeneratedSeriesTypes: boolean; |
There was a problem hiding this comment.
I also really like the way you named both properties in the issue example:
description and useDefaultSummary
they are generic and simple.
the word custom doesn't add much to that property meaning
markov00
left a comment
There was a problem hiding this comment.
LGTM all the changes are fine, just one last minor detail in the playground (you can also checkout the playground code from master so we don't introduce changes there at all)
.playground/playground.tsx
Outdated
| return ( | ||
| <div className="App"> | ||
| <Chart size={[500, 200]}> | ||
| <Settings customDescription="This is an area chart showing kibana sample data." /> |
There was a problem hiding this comment.
| <Settings customDescription="This is an area chart showing kibana sample data." /> | |
| <Settings description="This is an area chart showing kibana sample data." /> |
# [28.2.0](v28.1.0...v28.2.0) (2021-04-15) ### Bug Fixes * **xy:** consider `useDefaultGroupDomain` on scale config ([#1119](#1119)) ([c1b59f2](c1b59f2)), closes [#1087](#1087) ### Features * **a11y:** allow user to pass custom description for screen readers ([#1111](#1111)) ([2ee1b91](2ee1b91)), closes [#1097](#1097) * **partition:** add debuggable state ([#1117](#1117)) ([d7fc206](d7fc206)), closes [#917](#917)
|
🎉 This PR is included in version 28.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [28.2.0](elastic/elastic-charts@v28.1.0...v28.2.0) (2021-04-15) ### Bug Fixes * **xy:** consider `useDefaultGroupDomain` on scale config ([opensearch-project#1119](elastic/elastic-charts#1119)) ([269ff1a](elastic/elastic-charts@269ff1a)), closes [opensearch-project#1087](elastic/elastic-charts#1087) ### Features * **a11y:** allow user to pass custom description for screen readers ([opensearch-project#1111](elastic/elastic-charts#1111)) ([a0020ba](elastic/elastic-charts@a0020ba)), closes [#1097](elastic/elastic-charts#1097) * **partition:** add debuggable state ([opensearch-project#1117](elastic/elastic-charts#1117)) ([08f8baf](elastic/elastic-charts@08f8baf)), closes [opensearch-project#917](elastic/elastic-charts#917)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 72.22% 72.68% +0.46%
==========================================
Files 387 404 +17
Lines 12007 12353 +346
Branches 2623 2677 +54
==========================================
+ Hits 8672 8979 +307
- Misses 3310 3341 +31
- Partials 25 33 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Fixes #1097
This PR allows the user of elastic charts to pass in a
descriptionvia the<Settings />component (only for xy_charts) to be read by a screen reader.In the case where users descriptions are informational enough, they are able to disable the default generated series types through
useDefaultSummary, which by default is true.Checklist
Delete any items that are not applicable to this PR.
a11y_custom_descriptionwithin test_cases to test out with VoiceOver on Safari.)