-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Upstream Doughnut Chart to RC #1128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 ? |
4a2bc1f
to
2f12684
Compare
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. |
2f12684
to
e7b68ce
Compare
@advl , could you give me some examples of the edge cases you are considering? :) We also have a legend for the doughnut chart which could be triggered and autogenerated by a prop? Let me know your thoughts :). |
e7b68ce
to
49f0a88
Compare
7ab6c53
to
cbc1a9d
Compare
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. |
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 <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> |
7a03b0b
to
8816204
Compare
@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 :). |
8816204
to
674893c
Compare
Signed-off-by: Nkeiruka <[email protected]>
674893c
to
064f6a4
Compare
🎉 This PR is included in version 1.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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