Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sunburst chart #4037
Sunburst chart #4037
Changes from 11 commits
2bc3b9b
dba94f0
70f8cb2
dae528e
2ba7d32
8cac1ec
9ff90e2
5eddae3
78c92b5
8de8fc6
743602a
98aa5a3
7eb9f06
f1cafa2
4ac5e19
e532f59
0c16b53
306133e
50b4aa0
4e3ed73
034fe71
0d19149
8a38830
14fec41
7cdef4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Comments above each of these definitions will automatically land in storybook user facing documentation. We do not do this everywhere, but we totally should with all new Charts.
This will show up in the ArgsTable in the description column.
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.
seems like most of these already have default descriptions that fit pretty well, but I added comments for additional clarification. Let me know if anything is still unclear
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.
wonder if there are any accessibility concerns we can take care of up front? aria-labels, tabIndex, etc.?
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.
that's a good idea - I set these attributes on the g element that wraps each data point, and tested that the chart is now keyboard accessible
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.
The file is called hierarchy, the type used here is SunburstData though. Should this exist as a separate file?
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.
I think this file would make a lot of sense if we were to use a shared type between TreeMap and SunburstChart data? Is that possible / does it even make sense?
If not, I would recommend to integrate this data into the SunburstChart story.
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.
I think it would make sense to use the same type for both. But I think the
size
property should be re-namedvalue
since it could represent any quantitative value, and it should support a custom fill color.For now, I'll just delete this file and integrate the data into the sunburst story to avoid changing treemap.