feat(donut-chart): new module

[mikehobi]

Fixes #1956

• This PR contains CSS changes
• This PR does not contain CSS changes

Description

This PR adds donut-chart and chart-legend styling and documentation

Notes

Our implementation in eBayUI uses Highcharts

Screenshots

Donut Chart

Dark Mode

Light Mode

Chart Legend

Dark Mode

Light Mode

Checklist

• I verify the build is in a non-broken state
• I verify all changes are within scope of the linked issue

• I regenerated all CSS files under dist folder
• I tested the UI in all supported browsers
• I did a visual regression check of the components impacted by doing a Percy build and approved the build
• I tested the UI in dark mode and RTL mode
• I added/updated/removed Storybook coverage as appropriate

[changeset-bot]

🦋 Changeset detected

Latest commit: bcd7811

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@ebay/skin	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ArtBlue]

Hey @mikehobi , it looks like this is using Skin 17.3.0. Since we're now on 17.4.0, let's merge in master please. I'll review the PR afterwards.

[mikehobi]

Hey @mikehobi , it looks like this is using Skin 17.3.0. Since we're now on 17.4.0, let's merge in master please. I'll review the PR afterwards.

Done

[ArtBlue]

@mikehobi , I did a first round of review. One thing we should probably get done before we complete the full review is to fix the css flattening of selectors. I don't want to create additional comments on the unflattened lines since things are going to shift dramatically once you flatten the selectors.

I'll do another round of review once that's done to avoid you having to track things across diffs with major shifts.

[ArtBlue]

Even though the priorities were likely driven by DS, I'm wondering if being explicit about priorities on the implementation front is preferable here. There's value in implementing devs not to having to track things across components and not having to define them upfront.

If an implied priority is needed for visual differentiation, I'd think it would be easy enough to feed the chart data sorted by a preferred priority of data sets. With that approach, we wouldn't necessarily need explicit classes, something like this:

<dl>
    <div class="chart-legend__item">
        <dt class="chart-legend__label">Portion 1</dt>
        <dd class="chart-legend__value">10%</dd>
    </div>
    <div class="chart-legend__item">
        <dt class="chart-legend__label">Portion 2</dt>
        <dd class="chart-legend__value">20%</dd>
    </div>
    ...
</dl>

/* implied primary */
.chart-legend dl > .chart-legend__item:nth-child(1) {
    ...
}

/* implied secondary */
.chart-legend dl > .chart-legend__item:nth-child(2) {
    ...
}
...

Unless we need the priorities as a hook so the legend data can be linked to the chart eventually, we should consider the generalized approach.

[mikehobi]

@ArtBlue I like this!

One part that makes this tricky is that the order of colors changes due to total # of data points

For example the color order for two data points is ['secondary', 'primary'], while one single data point is just 'primary'

So would need some creative selectors to determine order

[ArtBlue]

Yikes! I'm not sure I like that order change specification. Maybe we need to reach out to DS and see if we can have consistency here. I'd imagine that as long as the data points visualized next to one another look good, which they should if they are all present, then there is no need to shuffle things.

[mikehobi]

@ArtBlue spoke with Randy, we can move forward with one set order for now. Made the update to use nth-child

[ArtBlue]

I suspect we may need some role to inform screenreader users of the intent of this grouping and likely also a legend title. As it stands, it's not clear what this block is meant to be. I'm not sure of this so let's hear from others.

[mikehobi]

I guess for standalone "chart-legend" usage this wouldn't have much context. But when used with the donut chart, the donut-chart heading would provide context?

From what I've read on dl, they just rely on a heading. Any thoughts here @cordeliadillon ?

[cordeliadillon]

I added a note below that the donut chart title should be a semantic heading.

What about using an aria-label on the DL, e.g. aria-label="{chart name} legend"?

So a full chart's accessibility tree would look something like this:

• H2: My cool donut chart
• Text: Various metrics
• {Graph accessibility tree}
• Description list: My cool donut chart legend
  • Term: Key 1
  • Definition: Value 1
  • ...

[ArtBlue]

Were the values really spec-ed to be right aligned? Per UX principles, these should probably be left-aligned to improve reading flow, consistency, scalability, and legibility. I believe it's also an a11y improvement for people with dyslexia.

[mikehobi]

I'm not sure I quite follow

I would consider this to be "tabular" format, in which numeric values are usually right-aligned

Here is the comp, it's basically emulating what a space-between would give if this was flex

Either case, yes to RTL support

[ArtBlue]

I think comps are sometimes "too perfect" in a sense that they relay the most optimistic visual scenario. When presented with the most pessimistic scenario, often design guidance changes. One of the only time right alignment is preferable on the web is for numeric data inside tables and there's a reason for that - tables usually have gridlines, which already creates visual separation and balance, which makes for good readability. Virtually all other text alignment (with minor exceptions) on the web is most optimal left-aligned. This is a general UX principle. See this for more info: https://beyondpixelss.medium.com/the-power-of-left-alignment-a-user-centric-perspective-in-design-8546ea330c71#

We may want to reach out to DS to get a revision to the specs.

[mikehobi]

I definitely agree that left-aligned text is generally more readable. I think in this context, since it's a value-pair, and not a large body of text, it doesn't seem to be too much of any issue. Maybe this is something we can discuss in a larger group.

For now, I've removed the text-align: right, and moved the flex to the label in order to achieve the styling (this removes the need for extra rtl specific styling

[ArtBlue]

If we end up keeping it this way (and I hope we don't), I believe we'd need an RTL treatment for this as well.

[cordeliadillon]

I think this should be a heading; difficult to say which heading level, as it matters how deeply nested it is. In the context of the Skin docs, this would be an H2.

[cordeliadillon]

I added a note below that the donut chart title should be a semantic heading.

What about using an aria-label on the DL, e.g. aria-label="{chart name} legend"?

So a full chart's accessibility tree would look something like this:

• H2: My cool donut chart
• Text: Various metrics
• {Graph accessibility tree}
• Description list: My cool donut chart legend
  • Term: Key 1
  • Definition: Value 1
  • ...