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 10 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.
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.
some of these values are pretty arbitrary, probably we should allow the user to pass in their own Text props or a custom Text component
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.
👍 We should evaluate all props to be maximum consistent with the existing API in other places.
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 the remaining props needed to make it consistent are dataKey, event handlers and animation props (if those are needed for this chart type). What is the dataKey used for?
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.
Given a dataset with multiple rows such as
[{a:1, b:2}. {a:3, b:4}]
The dataKey for example for a Line component identifies which row to use for the line values. The dataKey on the XAxis identifies which row to use for the xAxis ticks.
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.
got it - I added this in, so instead of just
value
, the user can pass any data keyThere 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.