feat(a11y): allow user to pass custom description for screen readers #1111
Conversation
rshen91
added
:accessibility
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.
Thank you both please see 3b13cc9
There was a problem hiding this comment.
Looks like you missed this file for renaming these variables
There was a problem hiding this comment.
😭 right, thanks! 76cc6b3
| @@ -78,6 +80,8 @@ export interface ReactiveChartStateProps { | |||
| 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; |
There was a problem hiding this comment.
Smart! Thank you 3b13cc9
| @@ -178,11 +184,19 @@ class XYChartComponent extends React.Component<XYChartProps> { | |||
| }} | |||
| // 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!
| @@ -78,6 +80,8 @@ export interface ReactiveChartStateProps { | |||
| 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!
| @@ -178,11 +184,19 @@ class XYChartComponent extends React.Component<XYChartProps> { | |||
| }} | |||
| // 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
.playground/playground.tsx
Outdated
|
|
||
| export class Playground extends React.Component { | ||
| render() { | ||
| 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)
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.)