Add support for customizable radius per sector in Pie Chart #5244

ilayda-cp · 2024-11-13T15:08:00Z

When creating a pie chart, users can currently specify only a single radius (outer radius), which applies uniformly to all pie sectors. This PR proposes allowing developers to define different radius sizes for each pie sector, enabling more customized and varied visualizations.

Description

A function type has been added to the outerRadius parameter. Users can now pass a function to outerRadius to calculate the radius dynamically. This function takes an element as a parameter and expects a number to be returned.

Related Issue

#5224

How Has This Been Tested?

Run the existing unit tests and added unit tests for the new feature.

Screenshots:

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 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

ckifer

We're going to need some unit tests to make sure this does what we say it does.

Also this might be better added to Cell rather than as a different data key.

@PavelVanecek thoughts?

ckifer · 2024-11-13T15:43:41Z

src/polar/Pie.tsx

        },
      ];
      const tooltipPosition = polarToCartesian(coordinate.cx, coordinate.cy, middleRadius, midAngle);
+      const outerRadiusParam = getValueByDataKey(entry, radiusKey, coordinate.outerRadius);


Does this no longer allow you to specify outerRadius as you currently can? I can't tell. But it should prefer outerRadius if set - otherwise use radiusKey

If you use both "outerRadius" and "radiusKey", the code first calculates the outer radius by using the "outerRadius" parameter. Then the "radiusKey" parameter is checked. If its provided it will use it otherwise it fallbacks to the calculated outerRadius. But i can definitely update it to prefer outerRadius parameter first.

One concern is how a user can define a reference radius for the pie chart if the sector radius is specified as a percentage. In this case, the outer radius parameter is used to calculate the sector's radius.

See my new comment above, I don't think this is the best way we can do this.

If we had the prop take a number or a function we wouldn't have to prefer one over the other

Not too sure what you mean by reference radius. Can you give me a scenario?

i agree on making outerRadius a function. It would make more sense. Ill update it

Have it allow both. A static number or percent like you can do today, or a function that returns a number.

Thanks! And thanks for being willing to change things a bit

made the changes

codecov · 2024-11-13T17:28:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.23%. Comparing base (1dedbc2) to head (3c5b111).
Report is 16 commits behind head on 3.x.

Additional details and impacted files

@@           Coverage Diff           @@
##              3.x    #5244   +/-   ##
=======================================
  Coverage   95.23%   95.23%           
=======================================
  Files         164      164           
  Lines       18654    18663    +9     
  Branches     4059     4061    +2     
=======================================
+ Hits        17765    17774    +9     
  Misses        883      883           
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

ckifer · 2024-11-14T06:50:27Z

src/polar/Pie.tsx

  className?: string;
  dataKey: DataKey<any>;
  nameKey?: DataKey<any>;
+  radiusKey?: DataKey<any>;


I also have a concern with the naming of this. There is currently both innerRadius and outerRadius props. Calling the new prop radiusKey might be confusing as to which radius we are affecting? And if you can do it to outerRadius why not inner as well?

Maybe instead of a new prop at all we should just let inner and outer radius take functions which give your data as an argument. Then you can do something like

outerRadius={(data) => data.someRadius}

That seems a lot simpler

ckifer · 2024-11-14T06:51:57Z

src/polar/Pie.tsx

        },
      ];
      const tooltipPosition = polarToCartesian(coordinate.cx, coordinate.cy, middleRadius, midAngle);
+      const outerRadiusParam = getValueByDataKey(entry, radiusKey, coordinate.outerRadius);


See my new comment above, I don't think this is the best way we can do this.

If we had the prop take a number or a function we wouldn't have to prefer one over the other

ckifer

nice! Few more comments

ckifer · 2024-11-14T17:27:56Z

src/polar/Pie.tsx

+  if (typeof item.outerRadius === 'function') {
+    outerRadius = item.outerRadius(element);
+  } else {
+    outerRadius = getPercentValue(item.outerRadius, maxPieRadius, maxPieRadius * 0.8);
+  }


nit: we could make this a small helper function so we don't have to use a let?

made the change

ckifer · 2024-11-14T17:28:57Z

src/polar/Pie.tsx

  innerRadius?: number | string;
  /** The outer radius of sectors */
-  outerRadius?: number | string;
+  outerRadius?: number | string | ((element: any) => number);


element implies "html element" or "react element". Can we name this something related to data?

dataItem, dataPoint, even just item would be ok

made the change

I may be missing something but I don't see the changes. Can you try to push again?

PavelVanecek

I'm okay with this change, better than the Cell. I have one request though, can you make the Pie accept a DataKey? That also allows functions, and more. We have a helper for resolving it so that should be easy change.

PavelVanecek · 2024-11-15T08:30:03Z

DataKey is this recharts-specific thing. It's either function or string or I forgot what else and we use it to allow users to decide how to pull primitives out of the data object.

Look at what other components do when they accept a dataKey prop. Basically I want this new feature to follow the same pattern.

PavelVanecek · 2024-11-15T08:30:51Z

I'm on phone on a train, I can paste some examples of what it should look like later when I get to the PC.

ilayda-cp · 2024-11-15T08:34:16Z

@PavelVanecek I checked it out, and the Pie component already has a dataKey prop. The value is extracted as follows:

const val = getValueByDataKey(entry, dataKey, 0);

This value is then passed to the function assigned to the outerRadius parameter (if its a function).

But do you mean you would like a 'key' (like data key) whose value would be used as the outer radius?

PavelVanecek · 2024-11-15T08:41:46Z

What I'm trying to say is, make the outerRadius also a DataKey type. So that one can decide how to decide the radius out of values of the data array.

PavelVanecek · 2024-11-15T08:43:33Z

const outerRadius = getValueByDataKey(entry, props.outerRadius, defaultOuterRadius)

Something like this.

ilayda-cp · 2024-11-15T08:44:07Z

this is similar to my first solution. I can revert back to that solution with some changes. What do you think @ckifer ?

PavelVanecek · 2024-11-15T08:46:47Z

Ah sorry for the back and forth. Maybe I should be more careful and read the history first.

Either way please don't make it use the Cell 😬 that's a blocker for us in the future and I will try to remove it in next versions.

ckifer · 2024-11-15T15:05:23Z

The problem with dataKey is it adds a bunch of props associated to each radius type. I guess that's fine and we shouldn't be scared of new props but this feels less intrusive.

If we want to add a dataKey down the road we still can, this essentially acts the same though

ckifer · 2024-11-19T07:29:19Z

@ilayda-cp just waiting on you to push your last commit on this one. Thanks!

ilayda-cp · 2024-11-20T05:28:06Z

@ckifer sorry about the delay pushed the change

ckifer

Two more tiny things but no worries about them. LGTM

ckifer · 2024-11-20T05:30:18Z

src/polar/Pie.tsx


+const getOuterRadius = (
+  dataPoint: any,
+  outerRadius?: number | string | ((element: any) => number),


Nit: for consistent interface this should be updated too

But it's not outward facing so all good

ckifer · 2024-11-20T05:31:03Z

src/state/selectors/pieSelectors.ts

  minAngle?: number;
  innerRadius?: number | string;
-  outerRadius?: number | string;
+  outerRadius?: number | string | ((element: any) => number);


Nit: Same here

ckifer · 2024-11-20T05:54:25Z

Thanks @ilayda-cp!

ilayda-cp added 2 commits November 13, 2024 15:36

feat:add adjustable radius to pie chart

3dfab21

feat:add adjustable radius to pie chart

baa7eac

ckifer requested changes Nov 13, 2024

View reviewed changes

ckifer requested a review from PavelVanecek November 13, 2024 15:46

Add tests for pie chart with adjustable radius

0c07f49

ckifer reviewed Nov 14, 2024

View reviewed changes

feat: update outerRadius param

2f649ea

ilayda-cp requested a review from ckifer November 14, 2024 12:52

ckifer requested changes Nov 14, 2024

View reviewed changes

update variable name

1b3ad2d

PavelVanecek requested changes Nov 15, 2024

View reviewed changes

ilayda-cp requested a review from ckifer November 15, 2024 08:26

feat:update outerRadius param

3c5b111

ckifer approved these changes Nov 20, 2024

View reviewed changes

ckifer merged commit 3cca3b0 into recharts:3.x Nov 20, 2024
5 checks passed

ilayda-cp deleted the pie-chart-with-step branch November 20, 2024 06:19

ckifer mentioned this pull request Nov 26, 2024

Variable Radius Pie Chart #2676

Closed

Uh oh!

Add support for customizable radius per sector in Pie Chart #5244

Add support for customizable radius per sector in Pie Chart #5244

Uh oh!

Conversation

ilayda-cp commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

Uh oh!

ckifer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckifer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PavelVanecek left a comment

Choose a reason for hiding this comment

Uh oh!

PavelVanecek commented Nov 15, 2024

Uh oh!

PavelVanecek commented Nov 15, 2024

Uh oh!

ilayda-cp commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PavelVanecek commented Nov 15, 2024

Uh oh!

PavelVanecek commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilayda-cp commented Nov 15, 2024

Uh oh!

PavelVanecek commented Nov 15, 2024

Uh oh!

ckifer commented Nov 15, 2024

Uh oh!

ckifer commented Nov 19, 2024

Uh oh!

ilayda-cp commented Nov 20, 2024

ilayda-cp commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

ilayda-cp commented Nov 15, 2024 •

edited

Loading

PavelVanecek commented Nov 15, 2024 •

edited

Loading