[charts] Support rounded corners on BarChart

[JCQuintas]

Initial Proposal

• We can use clip-path: inset() to easily make round corners

  • https://caniuse.com/css-clip-path
  • https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path

• Result https://codesandbox.io/p/sandbox/mui-mui-x-x-data-grid-forked-jt6jxh?file=%2Fsrc%2Fdemo.tsx%3A7%2C6

Docs

https://deploy-preview-12834--material-ui-x.netlify.app/x/react-charts/bars/#border-radius

resolves #12220
resolves #12947

Todo

• Support horizontal bars
• Support bars with both negative and positive values
• Support small bars (should we disable? decrease? figure a proportion?)
• Types documentation
• Examples

[mui-bot]

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

Updated pages:

• docs/data/charts/bars/bars.md

Generated by 🚫 dangerJS against 27a73ad

[alexfauquette]

Nice CSS property 🎉

When adding demos in the docs, we should probably link to the material design docs for educational purpose :)

The current strategy works for single bars, but not staked ones. For that you will need the information "is the bar the top one"

Otherwise, it will look like Nivo bar chart

The only solution I can think of is to go in the useAggregatedData and while looping on stacking group keep in memory the max/min coordinate of the group. And then maybe instead of applying the clip-path to the rectangle apply it to a group that contains all the stacked rectangles, such that having a rectangle with height=0 do not cause issue

Dividing the solution

We could go with two steps:

1. Document your solution for rounding bar charts. It's only one line, but it solves probably 80% of the usecases

slotProps={{ bar: { clipPath: `inset(0px round 5px 5px 0px 0px)` } }}

2. Introduce this notion of border radius that solves most of the edge cases described. For this one, I assume the borderRadius props should be moved to the BarPlot level instead of the BarElement.

Note

Surprisingly, chartjs when with full rounded rectangles

https://www.chartjs.org/docs/latest/samples/bar/border-radius.html

Echarts need to manually defines which rectangle is the end one.

[JCQuintas]

@alexfauquette can you take a look at the code again? Documentation is still missing, but I want to be sure I'm on the right path before documenting the behaviours.

https://codesandbox.io/p/sandbox/mui-mui-x-x-charts-rounded-corners-lvgmlc?file=%2Fsrc%2Fdemo.tsx%3A155%2C22

It took me a while to figure out how the mask could work with the animation, but the trick was to animate everything together 😅, there are some unnecessary masks, but I don't think they should be a problem.

There were two options I could go for in terms of masking

• Mask the entire column, approach a I chose.
• Mask only the last item in a column.

I chose to mask the entire column as I assume it gives more flexibility to the radius, as you can really make the columns round, although you probably shouldn't. If we put the mask only on the last item, if the item is very small or non-existant it would probably break and we would need more logic to figure everything out. Only drawbacks I see is that we have to use one mask for every segment, rather than one for the entire column, but it works and animation is aligned. Another "drawback" but this would be in any solution, is that if the "column" is smaller than the chosen radius, it will "clamp" the radius to the height of the column, which might look odd, but there is no other way around it.

[alexfauquette]

Looks quite good.

For now, all the bars are listed without structure, because it would require to create sub-components to have multiple useTransition

For mask we could rely on the id from SVG with the following structure:

// Assuming series a, b, c, d
// stacked together [a,b] and [c, d]
// With two x categories

// The clip paths of the stacked groups for first x value
<clipPath id="stack1-index0">
<clipPath id="stack1-index1">
// The clip paths of the stacked groups for second x value
<clipPath id="stack2-index0">
<clipPath id="stack2-index1">
...

// The rectangles for the first x value
<rect seriesId="a" clipId="stack1-index0">
<rect seriesId="b" clipId="stack1-index0">
<rect seriesId="c" clipId="stack2-index0">
<rect seriesId="d" clipId="stack2-index0">

// The rectangles for the second x value
<rect seriesId="a" clipId="stack1-index1">
<rect seriesId="b" clipId="stack1-index1">
<rect seriesId="c" clipId="stack2-index1">
<rect seriesId="d" clipId="stack2-index1">

[alexfauquette]

Why not directly puting the correct data?

x :Math.min(!mask.x ? Infinity : mask.x, result.x);
...

[JCQuintas]

Mostly so I don't repeat logic. All other values can change based on different reasons, so I would have to duplicate the logic.

This way if the object doesn't exist, I create the object in a step with default values, then in a second step I update the values based on the current iteration. If the object already exists, I just update the values based on the current iteration.

I find it simpler to organise my thoughts that way.

[alexfauquette]

A bit weird to have as many mask as bar. Would it make sens to define a <BarGroupClipPath clipingId={maskId} style={ ... } /> that defines the mask at put an id (will need to add the chartId to avoid conflicts)

Render those compoennts in their dedicated loop. And keep our current BarElement with clipPath={`url(#${maskId})`}

Bonus point for not rendering the BarGroupClipPath if borderRadius is undefined or is set to 0 :)

[JCQuintas]

My initial idea was to render all the masks, and only animate those. But it didn't seem visually correct, as the masks would "slide over" stacks, while currently stacks grow individually.

Another try was to have the statics masks, but allowing the stacks to grow. This solved the previous problem, but the stacks were "flat" (no border radius), while they were growing, which looked odd.

In the end I had to animate both the mask and the stack to have the behaviour I was looking for, but then I didn't know how to align the animations to play at exactly the same time, while adding/removing stacks also looked odd because each of the animations reacted differently.

My current solution simply directly animates them all in the same useTransition, with the sideeffect that we have as many masks as stack items, as you pointed out.

[JCQuintas]

I figured out I could simply use a new useTransition with the mask data 🙃, this way we can remove unneeded masks in when stacking

[JCQuintas]

from SVG with the following structure

codesandbox example

It doesn't work if we have multiple charts in the same page that have the same id/order. That was my first attempt actually. But svg ids follow the same rules as HTML, and are unique to the entire page.

I can just use useChartId which generates a unique id for each chart 😅

[JCQuintas]

@noraleonte @LukasTy I believe I addressed most if not all of @alexfauquette points. So please review when possible.

[joserodolfofreitas]

I loved how easy it is to apply a border-radius.
Can it support different values on different corners?

[JCQuintas]

I loved how easy it is to apply a border-radius. Can it support different values on different corners?

It is possible to implement, but I didn't, as I couldn't see a specific use-case for it.

[joserodolfofreitas]

It is possible to implement, but I didn't, as I couldn't see a specific use-case for it.

Regarding use cases, I think it's mainly about the design of your charts.

If it's a quick win, it'd be interesting if the borderRadius prop can also receive an array for when users need to be more specific.

<BarChart
  /* ... */
  borderRadius={[0, 10, 10, 0]}
/>

Great work, btw! 👌

[JCQuintas]

about the design of the charts

Yeah, but then, how does the overriding of the current behaviour work?

Like, if we use your example on the docs and suppose the array always overrides the default behaviour, we would get the following corners.

It could allow for other options however, like

Or 40px 10px 0px

[noraleonte]

This is looking so nice 🎉
Great work! I really enjoy how simple the dx is 🔥 Left a few nitpicks, but otherwise LGTM 🍰

Can't wait for this to get merged 🫢

[noraleonte]

@joserodolfofreitas I don't really see a use case for different borderRadius values 🤔 Considering that the current solution adapts to all chart types & layouts automatically. The only reason I could see this as justified is to have a different radius for positive/negative values, but even then it's a stretch 🙈

[LukasTy]

Looks great!
Good job! 👏 💯
Leaving some nitpick comments. 😉

[JCQuintas]

@LukasTy @noraleonte that for the good inputs. I'll also need one of you to accept the argos test 😅

Edit: nvm I can accept them myself, thanks @noraleonte

[joserodolfofreitas]

Sorry, I missed the thread here.
I should have been more emphatic because we should NOT limit customization to the use cases we can think of, especially when it's about visual customization with a style prop that already has a typical pattern in other spaces.

Recharts solution is overall worst than ours, but they cover this aspect, and maybe we can follow the same behavior.

especially when it's about visual customization

We should aim to be the most customizable as possible and dismantle any myth that we are not.

[JCQuintas]

@joserodolfofreitas I don't see many issues in implementing it, and it can be done without any breaking change. My point in this #12834 (comment) is more that currently the border radius manages Negative & Positive values differently, eg: positive vertical values have no border radius on the bottom corners. If we allow for setting border radius for every corner, just like the border-radius css, how should we override the current behaviour?

[joserodolfofreitas]

Alright, I created a new issue to host the discussion around a potential solution.
#13077