feat: Upstream Doughnut Chart to RC#1128
Conversation
edlerd
left a comment
There was a problem hiding this comment.
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 <nkeiruka.whenu@canonical.com>
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-componentsrun:Install the resulting tarball in your project with:
QA steps
Percy steps
Fixes
Fixes: N/A