feat(a11y): accessible goal and gauge chart by rshen91 · Pull Request #1174 · elastic/elastic-charts

rshen91 · 2021-05-27T15:58:35Z

Summary

Goal chart are more accessible! By default, users of assistive technologies will be able to learn more about goal charts such as the minimum, maximum, target, current value, and label major and minor.

Details

Fixes #1160

Checklist

Unit tests were updated or added to match the most common scenarios

myasonik

👏 👏 👏

myasonik · 2021-06-23T17:38:31Z

packages/charts/src/components/accessibility/types.tsx

+      {validGoalChart && goalChartData && <dd>{`Minimum: ${goalChartData.minimum}`}</dd>}
+      {validGoalChart && goalChartData && <dd>{`Maximum: ${goalChartData.maximum}`}</dd>}
+      {validGoalChart && goalChartData && <dd>{`Target: ${goalChartData.target}`}</dd>}
+      {validGoalChart && goalChartData && <dd>{`Value: ${goalChartData.value}`}</dd>}


We should follow the same DOM structure for all the items on in the list: <dt>{category}</dt><dd>{dataPoint}</dd>. We can also probably simplify it so there's only one conditional at the same time.

Not tested but something like this should work:

Suggested change

{validGoalChart && goalChartData && <dd>{`Minimum: ${goalChartData.minimum}`}</dd>}

{validGoalChart && goalChartData && <dd>{`Maximum: ${goalChartData.maximum}`}</dd>}

{validGoalChart && goalChartData && <dd>{`Target: ${goalChartData.target}`}</dd>}

{validGoalChart && goalChartData && <dd>{`Value: ${goalChartData.value}`}</dd>}

{validGoalChart && goalChartData && <>

<dt>Minimum:</dt>

<dd>${goalChartData.minimum}</dd>

<dt>Maximum:</dt>

<dd>${goalChartData.maximum}</dd>

<dt>Target:</dt>

<dd>${goalChartData.target}</dd>

<dt>Value:</dt>

<dd>${goalChartData.}}</dd>

</>}

Thank you, this makes sense! 44b9eb5

myasonik · 2021-06-23T17:42:58Z

packages/charts/src/components/accessibility/label.tsx

+  labelId,
+  goalChartLabels,
+}: A11ySettings & ScreenReaderLabelProps) {
+  if (!label && !goalChartLabels?.majorLabel) return null;


Should we do a goalChartLabels.minorLabel check here too?

Yas - 44b9eb5 thanks!!

myasonik · 2021-06-23T19:42:37Z

packages/charts/src/components/accessibility/label.tsx

+  const goalChartLabelsSection = !goalChartLabels?.majorLabel
+    ? null
+    : `Goal chart label: ${goalChartLabels.majorLabel} ${goalChartLabels.minorLabel}`;
+
+  return (
+    <Heading id={labelId}>
+      {label}
+      {goalChartLabelsSection}
+    </Heading>


I think this whole "while label to render" thing is, unfortunately, going to be more complicated...

So, first, the easy change:
The minorLabel we can just render as a <p> under the heading as there isn't a great semantic way to to signify a subheading but also shoving it all into a heading seems incorrect. So the DOM will look something like this:

return <> {unifedLabel && <Heading id={labelId}> {unifiedLabel} </Heading> } {goalChartLabels.minorLabel && <p>{goalChartLabels.minorLabel}</p>} </>;

Now we've got the more difficult change that I hid under a new variable I made up called unifiedLabel.

Goal charts are kind of great in that they're one of the few that have a visible label built in. I imagine a separate accessible label doesn't need to actually be passed in very often so let's hide that bit of indirection from users.

If both an accessible label and a chart label are passed in, just from what I see users doing a lot, I'd bet they will often be identical so let's check for that too before printing both.

Finally, only if both are supplied and are different should we print both and make clear what's what.

let unifiedLabel = ''; if (!label && goalChartLabels?.majorLabel) unifiedLabel = goalChartLabels?.majorLabel; else if (label && !goalChartLabels?.majorLabel) unifiedLabel = label; else if (label && goalChartLabels?.majorLabel && label !== goalChartLabels.majorLabel) { unifiedLabel = `${label}; Chart visible label: ${goalChartLabels.majorLabel}`; }

👆 I also changed the string a little bit. Because we don't know what the user might be passing in, and it seems a little strange to pass in both, I'm being super explicit about it but also trying not to duplicate info (we elsewhere say it's a goal chart).

Hope this all makes sense! Happy to zoom about it too!

I think I'm following!! 44b9eb5 for changes, thank you!

myasonik

Code LGTM, will test this next week before giving it a green light

myasonik · 2021-06-24T22:24:37Z

packages/charts/src/components/accessibility/types.tsx

+      <dd id={defaultSummaryId}>{chartTypeDescription}</dd>
+      {validGoalChart && goalChartData ? (
+        <>
+          <dt>{'Minimum: '}</dt>


Do we need the extra space at the end here? I don't think screen readers need it and I think this is hidden visually.

Ah good catch thank you 4f8e86c

myasonik

LGTM! 🚀

myasonik · 2021-06-29T19:45:14Z

packages/charts/src/components/accessibility/label.tsx

+    <>
+      {unifiedLabel && (
+        <Heading id={labelId}>
+          {label}


Suggested change

{label}

Can delete, the unifiedLabel includes it

Thanks!!! Good catch 7ad9893

nickofthyme

LGTM

# [31.1.0](v31.0.0...v31.1.0) (2021-07-06) ### Bug Fixes * **heatmap:** pick correct brush end value ([#1230](#1230)) ([57678fe](57678fe)), closes [#1229](#1229) ### Features * **a11y:** accessible goal and gauge chart ([#1174](#1174)) ([ffa8822](ffa8822)), closes [#1160](#1160)

nickofthyme · 2021-07-06T12:44:32Z

🎉 This PR is included in version 31.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [31.1.0](elastic/elastic-charts@v31.0.0...v31.1.0) (2021-07-06) ### Bug Fixes * **heatmap:** pick correct brush end value ([opensearch-project#1230](elastic/elastic-charts#1230)) ([cb95a75](elastic/elastic-charts@cb95a75)), closes [opensearch-project#1229](elastic/elastic-charts#1229) ### Features * **a11y:** accessible goal and gauge chart ([#1174](elastic/elastic-charts#1174)) ([775dc98](elastic/elastic-charts@775dc98)), closes [opensearch-project#1160](elastic/elastic-charts#1160)

rshen91 changed the title ~~(a11y) Accessible goal chart~~ feat(a11y): Accessible goal chart May 27, 2021

rshen91 added :accessibility Accessibility related issue :goal/gauge (old) Old Goal/Gauge chart related issues labels May 27, 2021

rshen91 added 3 commits June 22, 2021 07:31

feat: initial commit

91bb738

test: update snapshot

c63b88b

fix: merge changes

a105604

rshen91 force-pushed the a11y-goal branch from 3adaf49 to a105604 Compare June 22, 2021 13:34

feat: add goal chart data and labels

6245b05

rshen91 added the enhancement New feature or request label Jun 22, 2021

rshen91 added 2 commits June 22, 2021 09:09

test: add test

0dc89a6

fix: remove min max target value from not goal types

ebd8af7

rshen91 marked this pull request as ready for review June 22, 2021 16:54

rshen91 requested a review from myasonik June 22, 2021 16:54

rshen91 changed the title ~~feat(a11y): Accessible goal chart~~ feat(a11y): accessible goal chart Jun 23, 2021

rshen91 changed the title ~~feat(a11y): accessible goal chart~~ feat(a11y): accessible goal and gauge chart Jun 23, 2021

rshen91 mentioned this pull request Jun 23, 2021

feat(a11y): allow user to add optional semantic meaning to goal/gauge charts #1218

Merged

6 tasks

fix: clean up code

9d5bc31

myasonik reviewed Jun 23, 2021

View reviewed changes

rshen91 added 2 commits June 24, 2021 07:37

fix: code review changes

44b9eb5

Merge remote-tracking branch 'upstream/master' into a11y-goal

c65eb86

rshen91 requested a review from myasonik June 24, 2021 13:40

Merge remote-tracking branch 'upstream/master' into a11y-goal

9216fc8

myasonik reviewed Jun 24, 2021

View reviewed changes

rshen91 added 2 commits June 28, 2021 07:00

fix: fix code review

4f8e86c

test: update goal test

9b4d573

rshen91 requested a review from myasonik June 28, 2021 13:44

rshen91 added 2 commits June 29, 2021 07:13

Merge remote-tracking branch 'upstream/master' into a11y-goal

eb6918c

Merge remote-tracking branch 'upstream/master' into a11y-goal

16fe794

myasonik approved these changes Jun 29, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into a11y-goal

08c9c0b

fix: remove unnecessary code

7ad9893

nickofthyme approved these changes Jun 30, 2021

View reviewed changes

rshen91 merged commit ffa8822 into elastic:master Jun 30, 2021

rshen91 deleted the a11y-goal branch June 30, 2021 13:07

nickofthyme added the released Issue released publicly label Jul 6, 2021

-      {validGoalChart && goalChartData && <dd>{`Minimum: ${goalChartData.minimum}`}</dd>}
-      {validGoalChart && goalChartData && <dd>{`Maximum: ${goalChartData.maximum}`}</dd>}
-      {validGoalChart && goalChartData && <dd>{`Target: ${goalChartData.target}`}</dd>}
-      {validGoalChart && goalChartData && <dd>{`Value: ${goalChartData.value}`}</dd>}
+	  {validGoalChart && goalChartData && <>
+	    <dt>Minimum:</dt>
+	    <dd>${goalChartData.minimum}</dd>
+	    <dt>Maximum:</dt>
+	    <dd>${goalChartData.maximum}</dd>
+	    <dt>Target:</dt>
+	    <dd>${goalChartData.target}</dd>
+	    <dt>Value:</dt>
+	    <dd>${goalChartData.}}</dd>
+	  </>}

Conversation

rshen91 commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Checklist

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Jul 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rshen91 commented May 27, 2021 •

edited

Loading