[charts] PieChart onClick prop in the series data and demo #10506

giladappsforce · 2023-09-28T13:32:41Z

Added a new prop to PieChart series data called onClick, it's propagated to the underlying SVG element which triggers it and sends back the item that was clicked.

also added demo section to show this behavior.

[✅ ] I have followed (at least) the PR section of the contributing guide.

giladappsforce · 2023-09-28T13:34:44Z

#10507

mui-bot · 2023-09-28T13:36:57Z

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

Updated pages:

docs/data/charts/pie-demo/pie-demo.md

Generated by 🚫 dangerJS against 9a88b90

alexfauquette

Thanks for your nice contribution 👍. I'm just concerned by how it would work with other charts that don't use an object per item such as bar charts

alexfauquette · 2023-09-29T09:22:12Z

docs/data/charts/pie-demo/OnSeriesItemClick.tsx

+      <PieChart
+        series={[
+          {
+            data: [
+              { id: 0, value: 10, label: 'series A', onClick: setClickedItem },
+              { id: 1, value: 15, label: 'series B', onClick: setClickedItem },
+              { id: 2, value: 20, label: 'series C', onClick: setClickedItem },
+            ],
+          },
+        ]}
+        width={400}
+        height={200}
+      />


What would you think about this way of doing it? This is how Nivo and Rechart do it, and I think it scales better with other charts.

Suggested change

<PieChart

series={[

{

data: [

{ id: 0, value: 10, label: 'series A', onClick: setClickedItem },

{ id: 1, value: 15, label: 'series B', onClick: setClickedItem },

{ id: 2, value: 20, label: 'series C', onClick: setClickedItem },

],

},

]}

width={400}

height={200}

/>

<PieChart

series={[

{

data: [

{ id: 0, value: 10, label: 'series A' },

{ id: 1, value: 15, label: 'series B' },

{ id: 2, value: 20, label: 'series C' },

],

},

]}

onClick={setClickedItem}

width={400}

height={200}

/>

I think it could return 3 arguments: PieItemIdentifier is a small object that identifies the item. The item (what you are already passing) and maybe the event

I don't know if PieItemIdentifier and the item could be fusion. I such a case, it would be a nice signature (data, event) => void

I agree that it would be more flexible
I just wonder if we should name it onItemClik (or similar) and keep onClick for the root node event.

Concerning the signature, the core puts the event on the 1st param, might make sense to follow the same nomenclature here

Both recharts and nivo use onClick, and in 99% of the use case you want to trigger action on item click.

If you reach a level of customisation where you need to get click from the surface, you might want to use composition. Then it will be possible to do to the following

<ChartContainer onClick={handleSurfaceClick}> <BarPlot onClick={handleBarClick} /> <LinePlot onClick={handleLineClick} /> </ChartContainer>

I agree with the event/data order

Moved onclick handler to root component and added Identifier and event

alexfauquette

Thanks for the update. I added two commits.

One for minor modifications:

put native events first
use pieItemIdentifier typing
some small code cleaning

And a second one to improve the demonstration. If it's OK for you we can merge as it is

alexfauquette · 2023-10-02T13:44:05Z

packages/x-charts/src/PieChart/PieArc.tsx

@@ -66,7 +66,7 @@ const PieArcRoot = styled('path', {
 }));

 export type PieArcProps = Omit<PieArcOwnerState, 'isFaded' | 'isHighlighted'> &
-  Omit<React.ComponentPropsWithoutRef<'path'>, 'onClick'> &
+  React.ComponentPropsWithoutRef<'path'> &


Since we now have the custom onClick set at the <PiePlot/> level, we can use native event listener type

alexfauquette · 2023-10-02T13:47:01Z

packages/x-charts/src/PieChart/PiePlot.tsx

    event: React.MouseEvent<SVGPathElement, MouseEvent>,
+    pieItemIdentifier: PieItemIdentifier,
+    item: DefaultizedPieValueType,


I modified the order.

Instead of using the pie slice id which is not mandatory to simplify the creation of basics pie charts (maybe it should be), I used the PieItemIdentifier which contains:

The type of the series ('pie')

The series id

The index of the pie slice

At least having the series' id is necessary if you plot a pie chart with multiple pie like for example the two level pie example

alexfauquette · 2023-10-02T13:47:25Z

packages/x-charts/src/PieChart/PiePlot.tsx

+                    onClick={(event) =>
+                      onClick?.(event, { type: 'pie', seriesId, dataIndex: index }, item)


Just code simplification. Except if you see a drawback

The reason for the check is the cursor pointer, the check is currently if the onClick is undefined, with your change all pie slices have cursor pointer.

alexfauquette · 2023-10-02T13:47:47Z

packages/x-charts/src/PieChart/PiePlot.tsx

@@ -141,7 +138,6 @@ function PiePlot(props: PiePlotProps) {
                    outerRadius={outerRadius ?? availableRadius}
                    cornerRadius={cornerRadius}
                    id={seriesId}
-                    onClick={() => null}


I don't see why having it

alexfauquette · 2023-10-02T13:48:43Z

docs/data/charts/pie-demo/OnSeriesItemClick.js

+const items = [
+  { value: 10, label: 'series A ( no Id )' },
+  { id: 'id_B', value: 15, label: 'series B' },
+  { id: 'id_C', value: 20, label: 'series C' },
+];


I moved the item out of the demo definition since it is not an editable state

onSeriesItemClick prop and demo

Verified

This commit was signed with the committer’s verified signature.

tisonkun tison

GPG key ID: CD8564CB6A21E526

Verified
Learn about vigilant mode

a88678b

giladappsforce mentioned this pull request Sep 28, 2023

[charts] Allows to get clicks from pie slice #10507

Closed

giladappsforce added 3 commits September 28, 2023 16:44

format and fixed demo

Verified

This commit was signed with the committer’s verified signature.

tisonkun tison

GPG key ID: CD8564CB6A21E526

Verified
Learn about vigilant mode

9b6c02b

fix ts

1212a72

cursor pointer

f30bd42

zannager added the component: charts label Sep 28, 2023

alexfauquette reviewed Sep 29, 2023

View reviewed changes

giladappsforce and others added 2 commits October 1, 2023 09:33

Merge branch 'mui:master' into master

f484374

item identifier and event

0615e32

giladappsforce requested review from flaviendelangle and alexfauquette October 1, 2023 07:39

giladappsforce and others added 4 commits October 1, 2023 10:46

api docs

18a9b08

Merge branch 'mui:master' into master

a9c8d0f

update params order

86fd960

update the demo

9a88b90

alexfauquette approved these changes Oct 2, 2023

View reviewed changes

alexfauquette merged commit 1f77a73 into mui:master Oct 2, 2023

alexfauquette mentioned this pull request Oct 2, 2023

[charts] Pass onClick to chart item #10549

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] PieChart onClick prop in the series data and demo #10506

[charts] PieChart onClick prop in the series data and demo #10506

giladappsforce commented Sep 28, 2023 •

edited by alexfauquette

Loading

giladappsforce commented Sep 28, 2023

mui-bot commented Sep 28, 2023 •

edited

Loading

alexfauquette left a comment

alexfauquette Sep 29, 2023

flaviendelangle Sep 29, 2023

alexfauquette Sep 29, 2023 •

edited

Loading

giladappsforce Oct 1, 2023

alexfauquette left a comment

alexfauquette Oct 2, 2023

alexfauquette Oct 2, 2023

alexfauquette Oct 2, 2023

giladappsforce Oct 2, 2023

alexfauquette Oct 2, 2023

alexfauquette Oct 2, 2023

		onClick={(event) =>
		onClick?.(event, { type: 'pie', seriesId, dataIndex: index }, item)

[charts] PieChart onClick prop in the series data and demo #10506

[charts] PieChart onClick prop in the series data and demo #10506

Conversation

giladappsforce commented Sep 28, 2023 • edited by alexfauquette Loading

giladappsforce commented Sep 28, 2023

mui-bot commented Sep 28, 2023 • edited Loading

alexfauquette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladappsforce commented Sep 28, 2023 •

edited by alexfauquette

Loading

mui-bot commented Sep 28, 2023 •

edited

Loading

alexfauquette Sep 29, 2023 •

edited

Loading