feat: Upstream Doughnut Chart to RC

[Kxiru]

Done

• Added the Doughnut Chart Component

TODOs

• Correct failing test that verifies whether a tooltip shows on hover.

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

• Run Storybook
• Navigate to Doughnut Chart component

Percy steps

• List any expected visual change in Percy, or write something like "No visual changes expected" if none is expected.

Fixes

Fixes: N/A

[webteam-app]

Demo

Jenkins

demos.haus

[edlerd]

This is a good start. Some ideas to improve:

• The linter has found one issue
• In storybook there is an error on the Docs and Template section for the component
• For the test you can start easy i.e. by rendering a chart with two equal segments and checking the radius is calculated correctly as 180 degrees. Or be creative for other test cases.

[advl]

It looks good to me. I second @edlerd 's comments.

You could maybe also add additional stories with edge cases (100%, 0%). What do you think ?

Also, in general, and outside the scope of this PR, how do you think @Kxiru we could approach integrating more charts into the library ? Do you have an idea what other use cases might be, and how we could implement a common API ?

[Kxiru]

Apologies, it seems as though my existing test suite and much of my changes did not actually push 💀 . Here is the latest version, and I will address any outstanding suggestions on top of this.

Edit: @edlerd , as you can see, one of my tests is failing and this is the one regarding the Tooltips showing. I'd like to perfect it but Jest simply isn't having it, haha. Any ideas on what could be wrong with my current implementation? Alternatively, if you do not think that it is a necessary test to have, I can remove it.

[Kxiru]

@advl , could you give me some examples of the edge cases you are considering? :)
Also, I would be happy to implement some more graphs! I definitely think that some data visualisation components in React Components would go a long way for future projects. Two that we have perfected in the backlog include Multi-Bar (supporting Single bar) meters, and Horizontal bar charts.

We also have a legend for the doughnut chart which could be triggered and autogenerated by a prop?

Let me know your thoughts :).

[jmuzina]

Looks good as-is, but I wonder if there is a way for us to make the chart a bit more accessible - screen readers and keyboard users may have a hard time navigating and understanding this chart. I'll take a brief look and let you know if there's anything that can be done quickly in scope of just upstreaming this

[advl]

Agreed @jmuzina . I would suggest to timebox it and merge to unblock.
We can use these learnings for the new arch in case they would require too much work here.

[jmuzina]

Looks good as-is, but I wonder if there is a way for us to make the chart a bit more accessible - screen readers and keyboard users may have a hard time navigating and understanding this chart. I'll take a brief look and let you know if there's anything that can be done quickly in scope of just upstreaming this

A quick solution would be to give the chart a title and description, that way screen readers announce the contents. You can do this by creating <title> and <desc>.

 <svg
    className="doughnut-chart__chart"
    id={id.current}
    viewBox={`0 0 ${canvasSize} ${canvasSize}`}
    data-testid={TestIds.Section}
    aria-labelledby={`${id.current}-chart-title ${id.current}-chart-desc`}
  >
    {label && <title id={`${id.current}-chart-title`}>{label}</title>}
    <desc id={`${id.current}-chart-desc`}>
      {segments.map(
        (segment) => {
          let description = "";
          if (segment.tooltip) description += `${segment.tooltip}: `;
          description += segment.value;
          return description;
        })
        .join(",")}
    </desc>

This causes a screen reader to announce the chart contents:

[jmuzina]

Suggested change

	key={i}
	key={i}
	tabIndex={0}
	aria-label={`${tooltip}: ${value}`}

If you do something like this, the segments will also be tab-selectable and announce their contents to screen readers.

[jmuzina]

It would also be nice to make the tooltip of a focused segment visible and highlight the segment as you do with mouseover. Though that may be a bit more difficult and to be tackled as a separate issue.

[pastelcyborg]

Nit: Can we use a more descriptive name here?

[pastelcyborg]

I believe conventionally this is referred to as "thickness," rather than "width," which I think is a bit clearer.

[pastelcyborg]

Why is this necessary, and why is the tooltip child inline-block by default?