[charts-pro] Add a `borderRadius` property to `FunnelChart` #17660

JCQuintas · 2025-05-02T16:55:48Z

Adds a new borderRadius property to FunnelChart
Rewrite some curve implementations to gather all the points before drawing anything, as it makes calculating the border radius simpler.
Rename the FunnelStep curve to Step. It technically can handle any 4 point shape.

Changelog

Add a borderRadius property to FunnelChart. All funnels have a 8px borderRadius by default

This reverts commit 032abd7.

This reverts commit 81e03b7.

JCQuintas · 2025-05-02T16:56:50Z

packages/x-charts-pro/src/FunnelChart/FunnelPlot.tsx

@@ -20,6 +20,11 @@ export interface FunnelPlotProps extends FunnelPlotSlotExtension {
   * @default 0
   */
  gap?: number;
+  /**
+   * The radius, in pixels, of the corners of the funnel sections.
+   * @default 0


@noraleonte Should we make the default >0? Maybe also the gap? The funnel with border radius look quite nice to me 😄

Yes, I think we should. We can make it an 8 by default for a softer look 💙

mui-bot · 2025-05-02T16:58:49Z

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

Updated pages:

docs/data/charts/funnel/funnel.md

Generated by 🚫 dangerJS against bfab8a6

codspeed-hq · 2025-05-02T17:03:25Z

CodSpeed Performance Report

Merging #17660 will not alter performance

_{Comparing JCQuintas:funnel-border-radius (bfab8a6) with master (94211fd)}

Summary

✅ 8 untouched benchmarks

packages/x-charts-pro/src/FunnelChart/FunnelPlot.tsx

bernardobelchior · 2025-05-05T07:58:48Z

The border radius doesn't seem to be working:

JCQuintas · 2025-05-05T08:43:23Z

The border radius doesn't seem to be working:

I forgot to run scripts 🫠, locally they work cause they use the tsx file, on deploy they use js 😢

docs/data/charts/funnel/FunnelCurves.js

docs/data/charts/funnel/funnel.md

Co-authored-by: Bernardo Belchior <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>

alexfauquette · 2025-05-05T11:23:39Z

Not exactly about the borderRadius, but I missed the PR about gap

The math are correct, but it feels weird. For mee slices seems to be disconnected.

A solution I found is to do an average between the correct position and the one if we ignored the gap.

const xGap = point.x + (index === 0 || index === 3 ? this.gap : -this.gap);

return {
-   y: yGetter(xGap),
+   y: yGetter((point.x + xGap) / 2),

The reason why it looks better to me is the alignment.

With their current placement, the slices join in the middle of the gap, and my brain is not evolved enough to notice when lines cross a middle point.
With the second option, my brain realizes without difficulty that their is some alignement

I also noticed that doing the opposite (making the larges item align on the smaller one) seems weird too.

I went with an average, but that's impact somewhat the link between data and visualisation.

A simpler solution is to tweak params you pass to your interpolator as follow:

const xGetter = xFromY(
  slopeStart.x,
  slopeStart.y - this.gap,
  slopeEnd.x,
  slopeEnd.y - this.gap,
);

The idea is to modify the interpolator such that bottom points are the one actually drawn by the chart, but to point are targeting the one of the previous item:

Playing with those let me notice what's probably an error in slope start/end:

- const slopeStart = this.points.at(index <= 1 ? 0 : 2)!;
+ const slopeStart = this.points.at(index <= 1 ? 0 : 3)!;
- const slopeEnd = this.points.at(index <= 1 ? 1 : 3)!;
+ const slopeStart = this.points.at(index <= 1 ? 0 : 3)!;

Other

By the way, I'm not a big fan of the fact that gap adds space between the initial/final itams and the drawing area. For me it should only add space between items. What paddingInner does in D3JS But I don't see an easy way to do that correctly 🤔

JCQuintas · 2025-05-05T11:38:10Z

I went with an average, but that's impact somewhat the link between data and visualisation.

A simpler solution is to tweak params you pass to your interpolator as follow:
const xGetter = xFromY(
  slopeStart.x,
  slopeStart.y - this.gap,
  slopeEnd.x,
  slopeEnd.y - this.gap,
);

Will try this out. This will probably only affect this curve 🤔, so might be ok to do. I have pyramid and step-pyramid curves lined for review after this PR 😆

Playing with those let me notice what's probably an error in slope start/end:

- const slopeStart = this.points.at(index <= 1 ? 0 : 2)!;
+ const slopeStart = this.points.at(index <= 1 ? 0 : 3)!;
- const slopeEnd = this.points.at(index <= 1 ? 1 : 3)!;
+ const slopeStart = this.points.at(index <= 1 ? 0 : 3)!;

Why would the original be wrong? Points are:

vertical

3 ----- 0
 \     /
  2 - 1

horizontal

0 
| \
|  1
|  |
|  2
| /
3

So slopes are always 0-1 and 2-3 no?

Other

By the way, I'm not a big fan of the fact that gap adds space between the initial/final itams and the drawing area. For me it should only add space between items. What paddingInner does in D3JS But I don't see an easy way to do that correctly 🤔

Yeah, I couldn't offset that easily. Gap uses pixel, while the paddingInner implementation has the problem that it doesn't support pixels, which would give us the same issues we have in the bar, where we cant control the exact spacing.

alexfauquette · 2025-05-05T12:53:02Z

I have pyramid and step-pyramid curves lined for review after this PR 😆

Pyramids aren't an issue because they all align on the same line :)

So slopes are always 0-1 and 2-3 no?

There is a mistake in my diff

But the current implementation is

current	correct
0 -> 1	0 -> 1
2 -> 3	3 -> 2

Always starting from the larger to smaller edge.

By the way If you can make your ASCII scheme in the codebase that would be nice :)

JCQuintas · 2025-05-05T13:11:26Z

Always starting from the larger to smaller edge.

Doesn't seem like it affects the output of the graph

JCQuintas · 2025-05-05T13:12:16Z

By the way If you can make your ASCII scheme in the codebase that would be nice :)

I'm not sure where to put it, since it is true for all funnel curves

alexfauquette · 2025-05-05T13:45:18Z

Doesn't seem like it affects the output of the graph

Try the following interpolation ;)

xFromY(
  slopeStart.x,
  slopeStart.y - this.gap,
  slopeEnd.x,
  slopeEnd.y - 10 * this.gap,
);

bernardobelchior

I think the previous funnel chart looked better (Argos). Here's the current state:

The imaginary line connecting the 180 section to the 200 section doesn't seem to line up, which looks a bit weird to me 🤔

Previously, I think it looked better:

If it's just me, it's fine to go ahead with this PR.

docs/data/charts/funnel/funnel.md

bernardobelchior · 2025-05-06T10:13:21Z

packages/x-charts-pro/src/FunnelChart/curves/linear.ts

+    const getBorderRadius = () => {
+      if (this.gap > 0) {
+        return this.borderRadius;
      }
-    }
+      if (this.position === 0) {
+        return [0, 0, this.borderRadius, this.borderRadius];
+      }
+      if (this.position === this.sections - 1) {
+        return [this.borderRadius, this.borderRadius];
+      }
+      return 0;
+    };


Should we move this to a private class method? It doesn't reference any variable in scope.

bernardobelchior · 2025-05-06T10:16:14Z

packages/x-charts-pro/src/FunnelChart/curves/step.ts

+    });
+
+    // Add gaps where they are needed.
+    this.points = this.points.map((point, index) => {


Suggested change

});

// Add gaps where they are needed.

this.points = this.points.map((point, index) => {

}).map((point, index) => {

// Add gaps where they are needed.

I prefer to leave it above the var assignment rather than in the fn

Not a deal breaker, but seems weird to me that we're assigning a variable just to re-assign it later.

packages/x-charts-pro/src/FunnelChart/curves/borderRadiusPolygon.ts

JCQuintas · 2025-05-06T10:33:08Z

I think the previous funnel chart looked better (Argos). Here's the current state:

The imaginary line connecting the 180 section to the 200 section doesn't seem to line up, which looks a bit weird to me 🤔

Previously, I think it looked better:

If it's just me, it's fine to go ahead with this PR.

I think both look ok. I might prefer the original, but I have also been looking at these for a long time 🤔

bernardobelchior · 2025-05-06T11:52:35Z

I think the previous funnel chart looked better (Argos). Here's the current state:

The imaginary line connecting the 180 section to the 200 section doesn't seem to line up, which looks a bit weird to me 🤔
Previously, I think it looked better:

If it's just me, it's fine to go ahead with this PR.

I think both look ok. I might prefer the original, but I have also been looking at these for a long time 🤔

Now that I look at it with the border radius, it looks much better. Looks good to me 👍

bernardobelchior

Nice job 👍

JCQuintas added 10 commits May 2, 2025 13:37

initial work

7c21f66

logic

1684bf1

rename

9db8c25

improve docs

df94cbb

changes yolo

81e03b7

doesnt work

032abd7

Revert "doesnt work"

678c2a5

This reverts commit 032abd7.

Revert "changes yolo"

c9bc9a9

This reverts commit 81e03b7.

simplify

aea349b

docs and scripts

abfe1e0

JCQuintas added new feature New feature or request component: charts This is the name of the generic UI component, not the React module! labels May 2, 2025

JCQuintas requested review from bernardobelchior, alexfauquette and noraleonte May 2, 2025 16:55

JCQuintas self-assigned this May 2, 2025

JCQuintas commented May 2, 2025

View reviewed changes

JCQuintas changed the title ~~[charts-pro] Add borderRadius property to FunnelChart~~ [charts-pro] Add a borderRadius property to FunnelChart May 2, 2025

JCQuintas added 2 commits May 2, 2025 19:07

Merge branch 'master' into funnel-border-radius

29203eb

close the bump path

bb32b28

JCQuintas commented May 2, 2025

View reviewed changes

packages/x-charts-pro/src/FunnelChart/FunnelPlot.tsx Outdated Show resolved Hide resolved

Move border radius to series

5e824f0

scripts

fb25b9c

Merge branch 'master' into funnel-border-radius

cc85c34

bernardobelchior reviewed May 5, 2025

View reviewed changes

Update docs/data/charts/funnel/funnel.md

d849488

Co-authored-by: Bernardo Belchior <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>

JCQuintas added 2 commits May 5, 2025 13:07

docs suggestions

a0c7527

scripts

80dfa12

Merge branch 'master' into funnel-border-radius

51d7c27

linear curve alignment

91d126c

JCQuintas added 3 commits May 5, 2025 17:10

invert second slope

fe4ffb2

Merge branch 'master' into funnel-border-radius

485a6e6

default to 8 px border radius

35fd033

bernardobelchior reviewed May 6, 2025

View reviewed changes

suggestions

bfab8a6

bernardobelchior approved these changes May 6, 2025

View reviewed changes

JCQuintas merged commit e9148f8 into mui:master May 6, 2025
22 checks passed

JCQuintas deleted the funnel-border-radius branch May 6, 2025 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts-pro] Add a `borderRadius` property to `FunnelChart` #17660

[charts-pro] Add a `borderRadius` property to `FunnelChart` #17660

JCQuintas commented May 2, 2025 •

edited

Loading

JCQuintas May 2, 2025

noraleonte May 6, 2025

mui-bot commented May 2, 2025 •

edited

Loading

codspeed-hq bot commented May 2, 2025 •

edited

Loading

bernardobelchior commented May 5, 2025

JCQuintas commented May 5, 2025

alexfauquette commented May 5, 2025 •

edited

Loading

JCQuintas commented May 5, 2025 •

edited

Loading

Other

alexfauquette commented May 5, 2025

JCQuintas commented May 5, 2025

JCQuintas commented May 5, 2025

alexfauquette commented May 5, 2025

bernardobelchior left a comment •

edited

Loading

bernardobelchior May 6, 2025

bernardobelchior May 6, 2025

JCQuintas May 6, 2025

bernardobelchior May 6, 2025

JCQuintas commented May 6, 2025

bernardobelchior commented May 6, 2025

bernardobelchior left a comment

[charts-pro] Add a borderRadius property to FunnelChart #17660

[charts-pro] Add a borderRadius property to FunnelChart #17660

Conversation

JCQuintas commented May 2, 2025 • edited Loading

Changelog

JCQuintas May 2, 2025

Choose a reason for hiding this comment

noraleonte May 6, 2025

Choose a reason for hiding this comment

mui-bot commented May 2, 2025 • edited Loading

codspeed-hq bot commented May 2, 2025 • edited Loading

CodSpeed Performance Report

Merging #17660 will not alter performance

Summary

bernardobelchior commented May 5, 2025

JCQuintas commented May 5, 2025

alexfauquette commented May 5, 2025 • edited Loading

Other

JCQuintas commented May 5, 2025 • edited Loading

Other

alexfauquette commented May 5, 2025

JCQuintas commented May 5, 2025

JCQuintas commented May 5, 2025

alexfauquette commented May 5, 2025

bernardobelchior left a comment • edited Loading

Choose a reason for hiding this comment

bernardobelchior May 6, 2025

Choose a reason for hiding this comment

bernardobelchior May 6, 2025

Choose a reason for hiding this comment

JCQuintas May 6, 2025

Choose a reason for hiding this comment

bernardobelchior May 6, 2025

Choose a reason for hiding this comment

JCQuintas commented May 6, 2025

bernardobelchior commented May 6, 2025

bernardobelchior left a comment

Choose a reason for hiding this comment

[charts-pro] Add a `borderRadius` property to `FunnelChart` #17660

[charts-pro] Add a `borderRadius` property to `FunnelChart` #17660

JCQuintas commented May 2, 2025 •

edited

Loading

mui-bot commented May 2, 2025 •

edited

Loading

codspeed-hq bot commented May 2, 2025 •

edited

Loading

alexfauquette commented May 5, 2025 •

edited

Loading

JCQuintas commented May 5, 2025 •

edited

Loading

bernardobelchior left a comment •

edited

Loading