Sunburst chart

[csdiehl]

Description

Adds an entirely new chart type, the Sunburst Chart - resolves #3842.

Note: this is a WIP. I've added a Typescript component for a responsive sunburst chart, a story to demonstrate the functionality, and mock data. I still need to add event handlers, and a tooltip - I wanted to give this a chance to be reviewed before changes grow too large, and I'm looking for guidance on the best way to integrate these elements to match the style of existing components.

Related Issue

#3842

Motivation and Context

This adds a popular and useful new hierarchical chart type that is not addressed by any existing component in the library.

How Has This Been Tested?

I have not added tests yet.

Screenshots (if appropriate):

Current state of the chart - responsive dimensions and labels, still needs interactivity.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• I have added a storybook story or extended an existing story to show my changes
• All new and existing tests passed.

[ckifer]

To be honest, this looks awesome. Super clean.

We could maybe factor some of the logic out into testable functions, but besides that and the ability to customize everything this looks great.

You will definitely face issues when integrating with the current Tooltip situation and ideally we don't fit the style of the existing components because it is mostly bad 😅. @PavelVanecek is working on moving to an approach using Context instead of the giant generator function which will make this a lot easier.

Until then I think we can focus on getting this part to a good spot

[ckifer]

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

[nikolasrieble]

👍 We should evaluate all props to be maximum consistent with the existing API in other places.

[csdiehl]

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?

[nikolasrieble]

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.

[csdiehl]

got it - I added this in, so instead of just value, the user can pass any data key

[csdiehl]

thanks for reviewing @ckifer! Makes sense - the tooltip integration was my biggest question. Looks like the API allows users to add a tooltip as a child component, but some charts, like treemap, also have their own tooltip functions.

I'll focus on addressing your comments and adding unit tests for what is already here.

[ckifer]

I would way we probably want closer to how Tree Map works, if you want to create a similar function in the mean time I think that would be fine

[nikolasrieble]

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.

[csdiehl]

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

[nikolasrieble]

The file is called hierarchy, the type used here is SunburstData though. Should this exist as a separate file?

[nikolasrieble]

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.

[csdiehl]

I think it would make sense to use the same type for both. But I think the size property should be re-named value 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.

[nikolasrieble]

I tested it more thoroughly and reviewed the whole PR in detail.
This is a great contribution, I do not see this interfering with any ongoing refactoring, and it adds direct user value.

Thank you for putting in the effort and time ❤️ 💪 Great work.

Please do finalise by fixing the ArgsTable as shown in the screenshot.

[nikolasrieble]

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.

[ckifer]

Looking good, added a few more comments

[ckifer]

wonder if there are any accessibility concerns we can take care of up front? aria-labels, tabIndex, etc.?

[csdiehl]

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

[csdiehl]

Ok @ckifer and @nikolasrieble thanks for reviewing - I think I addressed all your comments, but let me know if I missed anything! I updated the props in the documentation to reflect only those that are currently available on this component

I also added tooltip, following the general approach used in Treemap It's not in the exactly right location, but I think I got pretty close

[ckifer]

I wonder if we could release this as experimental somehow.. We don't currently have a process for that but

[ckifer]

Looks awesome! Wondering if we would do well to add some sort of caveat to the chart that it is new or experimental, but I say lets merge it!

We can release it next minor and have the caveat exist in the changelog.

OR we could not export it yet and test it internally a bit.

Amazing contribution btw

[ckifer]

After looking at the deployed storybook you're right about the Text for sure, though not sure what we can do except leave it to the users to address themselves.

The Tooltip is definitely off a bit, we can keep iterating! 🚀

[uson1x]

What a timing! We were just about to hunt for a graph library for sunburst charts and here our beloved recharts adding it at the same moment.

I'm curious, maybe you have some ideas on how we could add animations to it for when you change the data?

In our case, we have a single layer (no children) of data to show and when user clicks on one of the segments, we want to async load new data for this segment from server and replace the existing single layer with a new single layer (but new numbers and colors). Would be nice to do it with some nice animations, so it feels more fluid.

Soo... I don't assume you had plans on working on sunburst animations next? 😄
And I hoped maybe you could share some pointers on how we could attempt to add those ourselves?

Thank you so much!

[csdiehl]

thanks @ckifer! I agree - we can fix anything that comes up in a separate PR. I'll leave it up to you about when to release. Looking forward to contributing more to the library!

@uson1x there is an onClick prop - so you could pass a callback to that that updates your data. But as of right now, it wouldn't animate. I see that animations are supported in some other charts, so that's something we could likely add later.