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

rshen91 · 2021-06-23T13:01:44Z

For reviewers, please review #1174 first!

Summary

The bandLabels prop is now added to Goal charts. This is an optional prop.

For example, users can pass band labels with semantic values associated with each band:
['chilly', 'brisk', 'warm'].

Details

The idea of creating a separate prop on the Goal component was to not interfere with the rendering logic that the bands prop handles.

Issues

Closes #1161

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Whenever possible, please check if the closing issue is connected to a running GH project
Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

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

packages/charts/src/chart_types/goal_chart/renderer/canvas/connected_component.tsx

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

myasonik · 2021-06-30T18:26:45Z

stories/goal/25_goal_semantic.tsx

+      <Goal
+        id="spec_1"
+        subtype={subtype}
+        base={0}
+        target={260}
+        actual={170}
+        // doesn't mess with canvas_renderers.ts
+        // @ts-ignore
+        bands={bandsWithSemantics.flat().filter((val) => typeof val === 'number')}
+        ticks={[0, 50, 100, 150, 200, 250, 300]}
+        tickValueFormatter={({ value }: BandFillColorAccessorInput) => String(value)}
+        bandFillColor={({ value }: BandFillColorAccessorInput) => semanticBandFillColor(value)}
+        labelMajor="Revenue 2020 YTD  "
+        labelMinor="(thousand USD)  "
+        centralMajor="170"
+        centralMinor=""
+        config={{ angleStart: Math.PI, angleEnd: 0 }}
+        semanticValues={bandsWithSemantics}
+      />


I wasn't sure where to leave this comment but I figured here is as good a place as any.

If a user is passing in semantic data, I don't think they should also have to send in the old bands prop. Here it looks like you're able to deduce it programmatically so it'd be nice to do that for the consumer.

So I'm not sure if this would be best as a new prop like you have it, or whether to allow a different shape of data to be passed into bands so that you can pass in either type of data (semantic or not) into one prop. (I would love to see it all in one prop but that's just a personal preference.)

rshen91 · 2021-07-06T17:25:02Z

jenkins test this

monfera · 2021-07-07T08:23:31Z

The prop bandLabels isnt being picked up in b5a1364. I need to investigate why that's happening but also welcome any insights.

As it's not a review comment, we can't set it to "resolve", so doing it via a reply (we looked at it together)

monfera · 2021-07-07T08:36:49Z

stories/goal/25_goal_semantic.tsx

+const subtype = GoalSubtype.Goal;
+
+export const Example = () => {
+  const bandsWithSemantics = ['freezing', 'chilly', 'brisk'];


Thanks for the changes! Nit: the variable could be renamed to bandLabels, and semanticBandFillColor to bandFillColor, though it's not significant enough for using electricity for a new test run, only worth doing if there's some other change

monfera

Wondering if instead of

<dt>200</dt><dd>freezing</dd>
<dt>250</dt><dd>chilly</dd>
<dt>300</dt><dd>brisk</dd>

it'd be better to use

<dt>0 - 200</dt><dd>freezing</dd>
<dt>200 - 250</dt><dd>chilly</dd>
<dt>250 - 300</dt><dd>brisk</dd>

as the bands/colors render as ranges. The "from" value is the previous band value, except for the lowest band value, where it's the lowest value in the ticks array.

The visualization itself does not show band label texts, while the colors represent intervals, not single values. What do you think @rshen91 @markov00 @myasonik?

myasonik · 2021-07-07T11:52:02Z

it'd be better to use
<dt>0 - 200</dt><dd>freezing</dd>
<dt>200 - 250</dt><dd>chilly</dd>
<dt>250 - 300</dt><dd>brisk</dd>
as the bands/colors render as ranges. The "from" value is the previous band value, except for the lowest band value, where it's the lowest value in the ticks array.

++ I like it!

rshen91 · 2021-07-07T13:17:12Z

Wondering if instead of
<dt>200</dt><dd>freezing</dd>
<dt>250</dt><dd>chilly</dd>
<dt>300</dt><dd>brisk</dd>
it'd be better to use
<dt>0 - 200</dt><dd>freezing</dd>
<dt>200 - 250</dt><dd>chilly</dd>
<dt>250 - 300</dt><dd>brisk</dd>
as the bands/colors render as ranges. The "from" value is the previous band value, except for the lowest band value, where it's the lowest value in the ticks array.

The visualization itself does not show band label texts, while the colors represent intervals, not single values. What do you think @rshen91 @markov00 @myasonik?

I also think this is fine! 0bfdec4 for changes - thanks!

myasonik · 2021-07-07T16:08:37Z

In an effort to provide value to visual and screen reader users alike, I wonder if it makes sense to expose the semantic meaning in a tooltip over the ranges as well?

Color understanding isn't universal so having it explicitly defined for many users may be helpful.

rshen91 · 2021-07-07T17:19:39Z

Thanks Rachel, looks good! I don't remember if there's a guarantee that the first value is the lowest, though in practice that's probably always the case (we could add it to the documentation for ticks). Or safer, just use
return geoms.bulletViewModel.ticks.reduce((min: number, {value}: TickViewModel) => Math.min(min, value), Infinity);

Actually I'm seeing that the bulletViewModel includes a lowestValue. Is this value good for this case? Thanks!

monfera · 2021-07-07T22:04:51Z

Actually I'm seeing that the bulletViewModel includes a lowestValue. Is this value good for this case? Thanks!

Yes @rshen91 it's better than my suggestions, good call!

monfera · 2021-07-07T22:25:03Z

I agree @myasonik with showing band info in a visual channel. It'd need some design, eg.

Should the tooltip show all bands all the time, or only the currently hovered band? (in case of the latter, there's work needed toward capturing the specific band under the mouse; I'm working on a somewhat related PR which would be prerequisite for this)
Usually, it's worth allowing display without requiring the user to perform interactions, esp. if it's compact and there is space for a legend
Is the tooltip still useful if there's also legend support? I think yes, because a legend is not mandatory, and it also helps if some users have difficulty correlating colors (a hover gives a direct band name in the tooltip)

So,

as there are some choices, and
it has some blockers, yet this PR is a prerequisite of ongoing bullet tooltip feature work, due to big merge conflict from some refactoring (addressing [Bullet] Tooltips shouldn't appear on the entire canvas #1187), and
there's no drawback to merging this PR as it is, compared to adding this task,

the tooltip with tbe band names could be a separate PR. Pls. share if it has some negatives

monfera

LGTM!

# [32.0.0](v31.1.0...v32.0.0) (2021-07-12) ### chore * **license:** elastic license 2.0 ([#1242](#1242)) ([67fa0a3](67fa0a3)), closes [#1213](#1213) ### Features * **a11y:** allow user to add optional semantic meaning to goal/gauge charts ([#1218](#1218)) ([87629e2](87629e2)), closes [#1161](#1161) ### BREAKING CHANGES * **license:** the library is released now under Elastic License 2.0 and SSPL

nickofthyme · 2021-07-12T15:15:54Z

🎉 This PR is included in version 32.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nickofthyme · 2021-07-20T08:40:08Z

🎉 This PR is included in version 32.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [31.2.0](Metrika-Inc/metrika-charts@v31.1.0...v31.2.0) (2021-07-21) ### Bug Fixes * **heatmap:** correctly place x axis text in all cases ([1cf37d4](Metrika-Inc/metrika-charts@1cf37d4)) * **xychart:** when axis title is on top, it correctly lines up with axis labels vertically ([12839b5](Metrika-Inc/metrika-charts@12839b5)) ### Features * **a11y:** allow user to add optional semantic meaning to goal/gauge charts ([elastic#1218](https://github.com/Metrika-Inc/metrika-charts/issues/1218)) ([87629e2](Metrika-Inc/metrika-charts@87629e2)), closes [elastic#1161](https://github.com/Metrika-Inc/metrika-charts/issues/1161) * **heatmap:** added the ability to customize the position and rotation of the x axis labels ([8280b64](Metrika-Inc/metrika-charts@8280b64)) * **heatmap:** hard code x-axis labels sideways ([26ebe5e](Metrika-Inc/metrika-charts@26ebe5e)) * **xychart:** basic customization support for putting the axis title on top of the chart ([79865ba](Metrika-Inc/metrika-charts@79865ba)) * **xychart:** hard code left axis label to top of graph ([f10ff72](Metrika-Inc/metrika-charts@f10ff72)) * **xychart:** the title for the right axis can now also go on top ([b2087a1](Metrika-Inc/metrika-charts@b2087a1))

rshen91 added 7 commits June 22, 2021 07:31

feat: initial commit

91bb738

test: update snapshot

c63b88b

fix: merge changes

a105604

feat: add goal chart data and labels

6245b05

test: add test

0dc89a6

fix: remove min max target value from not goal types

ebd8af7

feat: add semantic meaning to goal/gauge charts

9c90b45

rshen91 changed the title ~~feat(a11y):Allow user to add optional semantic meaning to goal/gauge charts~~ feat(a11y): allow user to add optional semantic meaning to goal/gauge charts Jun 23, 2021

fix: chart api

37acc83

rshen91 added :accessibility Accessibility related issue :goal/gauge (old) Old Goal/Gauge chart related issues enhancement New feature or request wip work in progress labels Jun 23, 2021

rshen91 added 5 commits June 23, 2021 07:27

fix: api chart extract

503cb9d

test: add story snapshot

91f126b

feat: clean up semantic representation in goals rendering

f63c09c

refactor: add goal semantic description component

5620c16

test: add tests

aba9bae

rshen91 marked this pull request as ready for review June 23, 2021 16:26

rshen91 requested review from myasonik, monfera and nickofthyme June 23, 2021 16:26

nickofthyme reviewed Jun 23, 2021

View reviewed changes

packages/charts/src/components/accessibility/types.tsx Outdated Show resolved Hide resolved

style: clean up code per code review

7c4e432

rshen91 removed the wip work in progress label Jun 23, 2021

rshen91 requested a review from nickofthyme June 24, 2021 13:54

rshen91 added 2 commits June 30, 2021 08:28

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

0c77eec

style: fix test

3f0681b

myasonik reviewed Jun 30, 2021

View reviewed changes

fix: update per code review

250ab2c

rshen91 requested review from myasonik and monfera July 6, 2021 18:11

monfera reviewed Jul 7, 2021

View reviewed changes

monfera self-requested a review July 7, 2021 08:37

monfera reviewed Jul 7, 2021

View reviewed changes

refactor: code review

0bfdec4

rshen91 requested a review from monfera July 7, 2021 13:17

myasonik mentioned this pull request Jul 7, 2021

[Lens] Support bullet/gauge chart elastic/kibana#89859

Closed

rshen91 mentioned this pull request Jul 7, 2021

(a11y improvement & tooltip redesign) Add more semantic meaning to the tooltip if provided (goal) #1232

Closed

fix: pull lowest value

3702ecf

monfera approved these changes Jul 8, 2021

View reviewed changes

rshen91 merged commit 87629e2 into elastic:master Jul 8, 2021

rshen91 mentioned this pull request Jul 9, 2021

Add legend about bands/range with color picker #1236

Closed

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

monfera mentioned this pull request Jul 14, 2021

refactor(bullet): better tooltip foundations #1246

Merged

5 tasks

nickofthyme added the released on @32.x label Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rshen91 commented Jun 23, 2021 •

edited

Loading

myasonik Jun 30, 2021

rshen91 commented Jul 6, 2021

monfera commented Jul 7, 2021

monfera Jul 7, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

myasonik commented Jul 7, 2021

rshen91 commented Jul 7, 2021

myasonik commented Jul 7, 2021

rshen91 commented Jul 7, 2021

monfera commented Jul 7, 2021

monfera commented Jul 7, 2021 •

edited

Loading

monfera left a comment

nickofthyme commented Jul 12, 2021

nickofthyme commented Jul 20, 2021

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

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

Conversation

rshen91 commented Jun 23, 2021 • edited Loading

Summary

Details

Issues

Checklist

myasonik Jun 30, 2021

Choose a reason for hiding this comment

rshen91 commented Jul 6, 2021

monfera commented Jul 7, 2021

monfera Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

monfera left a comment • edited Loading

Choose a reason for hiding this comment

myasonik commented Jul 7, 2021

rshen91 commented Jul 7, 2021

myasonik commented Jul 7, 2021

rshen91 commented Jul 7, 2021

monfera commented Jul 7, 2021

monfera commented Jul 7, 2021 • edited Loading

monfera left a comment

Choose a reason for hiding this comment

nickofthyme commented Jul 12, 2021

nickofthyme commented Jul 20, 2021

rshen91 commented Jun 23, 2021 •

edited

Loading

monfera Jul 7, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

monfera commented Jul 7, 2021 •

edited

Loading