mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[charts] Funnel charts

Open JCQuintas opened this issue 1 year ago • 5 comments

Todo

  • [x] Initial working prototype
  • [x] Legend
  • [x] Click events
  • [x] Added new curves BumpX/BumpY that look better on funnels
  • [x] Display labels
  • [x] Highlights
  • [x] Tooltips
  • [x] Position labels
  • [x] Value formatter
  • [x] "Band/Static" sections (right now they scale based on value)
  • [x] "Piramid" chart (works out of the box, though not very piramidy)
  • [ ] "Invert" logic for true "piramid" behaviour
  • [ ] Move to Pro package 🙃
  • [ ] Move into its own ChartContainer and Providers
  • [ ] Explore creating a axis hook for config
  • [ ] Same-series works (current has the same color, wrong tooltip)
  • [ ] Slots
  • [ ] Stacking should be able to set one funnel on top of another
  • [ ] Refine props
  • [ ] Docs

#7929

JCQuintas avatar Oct 02 '24 14:10 JCQuintas

Deploy preview: https://deploy-preview-14804--material-ui-x.netlify.app/

Updated pages:

Generated by :no_entry_sign: dangerJS against a18cfff5c7ae4d4404886bfa52e5787ab94b063a

mui-bot avatar Oct 02 '24 14:10 mui-bot

CodSpeed Performance Report

Merging #14804 will not alter performance

Comparing JCQuintas:7929-chartsrfc-funnel-charts (a18cfff) with master (824047f)

Summary

✅ 6 untouched benchmarks
🆕 1 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 FunnelChart with big data amount N/A 24 ms N/A

codspeed-hq[bot] avatar Oct 02 '24 14:10 codspeed-hq[bot]

@alexfauquette I won't finish this before my time off, so when you got time, it would be nice to check my approach here. Specially the way I'm dealing with series.

Instead of having a series similar to PieSeriesType, it is like a regular series, but with stackId: 'defaultStackId' and multiple series to create a funnel stack.

I know there are complications to it, but it was an exploration to have "data" be simple, and work around series[].label being useless in pie charts, so I wanted to hear your thoughts on it 😅

JCQuintas avatar Oct 14 '24 12:10 JCQuintas

The implementation is clean. But I'm not a big fan of using the stackId for the funnel.

From a philosophical point of view, I consider funnel as a bar/line charts with less features (single series) but different visualization. Here is a funnel and the same data in a bar chart

image

Outside of the question of what is a funnel series, my main reason against it is: "You want to stay away from the stacking strategy" Joke aside, the staking strategy exists because bar and line charts have very close behavior, so it make sense to share it. But for staking series with only one value, it feels wrong

About the stuff you want to fix

work around series[].label being useless in pie charts

It's useless because we do not use it 😇 If we pick your double pie chart with Device and browser, we could use it by adding the series label on top. In this screenshot, we could see "Device" as a header of the tooltip. It would be even more useful with a sunburst.

image

By the way, having data as an array which expect only one value feels weird

{ label: 'first', data: [200] },
{ label: 'second', data: [150] },

alexfauquette avatar Oct 14 '24 14:10 alexfauquette

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Oct 31 '24 14:10 github-actions[bot]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Jan 20 '25 13:01 github-actions[bot]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Jan 27 '25 12:01 github-actions[bot]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Jan 29 '25 13:01 github-actions[bot]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Jan 30 '25 10:01 github-actions[bot]

In this example, the values of the X axis seem counterintuitive to me.

image

The data only has positive values and the labels show values from 0 to 100%, however the X axis is labeled from -100 to 100.

I thought a couple of options -- explained below --, but all of them have downsides, so I suppose that's why the current implementation was chosen:

  1. Update the X axis to range from -50% to 50%. This way, a label with 100% spans from -50% to 50%, and the chart is centered around 0%, however it looks weird because there are no negative percentages in the labels;
  2. Update the X axis to range from 0% to 100%. This would cause the chart not to be centered around 0%, but the scale of X axis is correct;

Curious to read your thoughts on this.

bernardobelchior avatar Feb 05 '25 11:02 bernardobelchior

In this example, the values of the X axis seem counterintuitive to me.

image

The data only has positive values and the labels show values from 0 to 100%, however the X axis is labeled from -100 to 100.

I thought a couple of options -- explained below --, but all of them have downsides, so I suppose that's why the current implementation was chosen:

  1. Update the X axis to range from -50% to 50%. This way, a label with 100% spans from -50% to 50%, and the chart is centered around 0%, however it looks weird because there are no negative percentages in the labels;
  2. Update the X axis to range from 0% to 100%. This would cause the chart not to be centered around 0%, but the scale of X axis is correct;

Curious to read your thoughts on this.

Yeah, simply inverting the sign on the values was the easiest way to center the funnel. The displayed axis is not the main focus of this chart, the idea of showing the top axis was mostly for the "horizontal" orientation, though I understand it could be confusing 🤔

A possible alternative would be to flip the sign in the data from one side only, so we would have a 100...0...100 range, instead of -100...0...100

JCQuintas avatar Feb 05 '25 12:02 JCQuintas

In this example, the values of the X axis seem counterintuitive to me. image The data only has positive values and the labels show values from 0 to 100%, however the X axis is labeled from -100 to 100. I thought a couple of options -- explained below --, but all of them have downsides, so I suppose that's why the current implementation was chosen:

  1. Update the X axis to range from -50% to 50%. This way, a label with 100% spans from -50% to 50%, and the chart is centered around 0%, however it looks weird because there are no negative percentages in the labels;
  2. Update the X axis to range from 0% to 100%. This would cause the chart not to be centered around 0%, but the scale of X axis is correct;

Curious to read your thoughts on this.

Yeah, simply inverting the sign on the values was the easiest way to center the funnel. The displayed axis is not the main focus of this chart, the idea of showing the top axis was mostly for the "horizontal" orientation, though I understand it could be confusing 🤔

A possible alternative would be to flip the sign in the data from one side only, so we would have a 100...0...100 range, instead of -100...0...100

Yeah, makes sense. I wonder if other libraries allow creating a funnel chart with an X axis.

bernardobelchior avatar Feb 05 '25 13:02 bernardobelchior

Can you make curves not so shart if you can make it like nivo it would be nice

thanks for the hard work

saver711 avatar Feb 05 '25 14:02 saver711

Can you make curves not so shart if you can make it like nivo it would be nice

thanks for the hard work

Hi, curves will be supported, you can see more in the current branch's documentation

https://deploy-preview-14804--material-ui-x.netlify.app/x/react-charts/funnel/#curve-interpolation

Screenshot 2025-02-05 at 15 20 25

JCQuintas avatar Feb 05 '25 14:02 JCQuintas

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Feb 07 '25 16:02 github-actions[bot]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Feb 24 '25 18:02 github-actions[bot]

@alexfauquette @bernardobelchior please take a look at this tentative api: https://github.com/JCQuintas/mui-x/pull/10/files

The idea is to remove the x/yAxis config. And replace it with categoryAxis, which is a simplified axis.

It is an issue to lose the x/y since it is based on the chart layout=horizontal/vertical. Which means the types are a bit shitty (eg: position can't be properly scoped)

Wanting to get some ideas on how this is going before I try to handle the value/aux axis

JCQuintas avatar Feb 25 '25 21:02 JCQuintas

@alexfauquette @bernardobelchior please take a look at this tentative api: JCQuintas/mui-x#10 (files)

The idea is to remove the x/yAxis config. And replace it with categoryAxis, which is a simplified axis.

It is an issue to lose the x/y since it is based on the chart layout=horizontal/vertical. Which means the types are a bit shitty (eg: position can't be properly scoped)

Wanting to get some ideas on how this is going before I try to handle the value/aux axis

In funnel charts, the categorical/discrete axis and continuous axis distinction makes sense. Even if the discrete axis receives a number, it will always display it as a label, so it's fine to make the distinction. This contrasts with, for example, a line chart where the x axis could, in theory, contain discrete or continuous data.

From the PR there's two things that I'd like to discuss.

First, in the linear scale example the API is categoryAxis={{ scaleType: 'linear' }}, which is a bit weird. How can a categorical axis have a linear scale? The answer is that the scale is actually uses the data from the continuous axis to scale the categorical axis. I think this is a bit weird, but if we call it y-axis we'd have exactly the same problem, only the prop would have a different name. So, not a blocker to me.

Second, it isn't clear to me how composition would work. Would we still need to render a ChartYAxis component to display the categorical axis or would it be a ChartCategoricalAxis?

bernardobelchior avatar Feb 26 '25 08:02 bernardobelchior

eg: position can't be properly scoped

That does not look like a big issue. devs should be able to understand that a vertical funnel can't have its categories displayed on top.

I was thinking about a way to make the series label and categories label be forced to be coherent with a single source of truth, but without success.

Overall, I think your funnel already has all the features we could expect from it, plus some additional edge cases. And you're currently trying to make those edge cases work together. If you look at the demos, now we have no mention of any axes up to the last two examples. One is to display the y-axis, and the other is to change the axis scale. I'm in favor of shipping the component as it is and collect feedback. Otherwise, we might work hard on details nobody uses, and miss trivial one

Small note: I realized that to introduce composition for the funnel we will need to introduce a ChartFunnelDataProvider to hide this internal logic between the series and axis to the user. But that can be left for future PRs

alexfauquette avatar Feb 26 '25 08:02 alexfauquette

@alexfauquette last question would be, do we release "as is" or as unsafe_?

JCQuintas avatar Feb 26 '25 14:02 JCQuintas

@alexfauquette last question would be, do we release "as is" or as unsafe_?

I'm in favor of an unstable release. Which includes:

  • Exporting the component as Unstable_
  • And in the docs nav bar set unstable: true

First to give it a try and have the freedom to do small breaking changes if needed according to user issues. And for the very small ones like having a gradual default color palette, or modifying highlight colors

alexfauquette avatar Feb 26 '25 14:02 alexfauquette