Closed Kxiru closed 1 month ago
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 ?
Apologies, it seems as though my existing test suite and much of my changes did not actually push :skull: . 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.
@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 :).
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
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.
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 and @pastelcyborg , thank you for your reviews and suggestions so far. I have implemented these and await another once over whenever you are ready :).
:tada: This PR is included in version 1.6.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
Done
TODOs
Correct failing test that verifies whether a tooltip shows on hover.QA
Storybook
To see rendered examples of all react-components, run:
QA in your project
from
react-components
run:Install the resulting tarball in your project with:
QA steps
Percy steps
Fixes
Fixes: N/A