[charts] Add `onClick` support #11411

alexfauquette · 2023-12-14T11:01:12Z

This PR is have commits per component impacted. I recommend to have first a look a documentation for the DX aspect, and a per commit review for code quality

Decision

I went with the onAxisClick/onItemClick such that easy usecase are handled with easy solution. The more complex ones are:

scatter plot for which I extend the the voronoid handler
line and bar chart that are the one using the onAxisClick

For docs, I'm moving to a docs per component to explain edge cases only in the docs of the charts for which they are relevant. It's easier to explain why onClick is more complex than usual in the scatter charts than in a dedicated page

It's a attempt to take best of the different proposed PRs

Current solution

I'm adding an <ChartsHandleOnClick onclick={onClick} /> that can be added anywhere.

This simply contain a useEffect that adds an event listener to the svgRef which will call the onClick callback with the event, plus some data if an item or an axis is active according to our internal state.

Why such a solution

The onClick is not passed to each element, but listen at the <svg/> level for multiple reasons:

For scatter charts, points are too small, so it's more interesting to benefit from the voronoid cells. And that can not be done when listening to event on components rendered by the scatter plot.
For stacked charts, or charts with band/point axis, it's not clear if you want the data of the item you clicked, or all the items at a given axis position. See the recording of PR [charts] On line chart click #10575 to get a good example about where axis data is more interesting than item ones.

Issues

The main drawback I can see is the mouse pointer.

The event being at the <svg/> level to modify the mouse pointer when there is something to click I would need to modify style by looking at the context to see if there is an interactive item or axis selected.
Since the onClick returns multiple values, I can not know if devs will use only item data, or axis data. So I can't put a meaning full style.

Ideas

I've some ideas of improvement

Idea 1

The first one is to distinguish item and axis click with distinct onItemClick and onAxisClick

This should allow to setup good default styling:

If onAxisClick is defined the drawing area get cursor: pointer
If onItemClick is defined the drawing area get cursor: pointer when the active item is not null.

Idea 2

A second one is to add onClick to element slots with additional data. Such that for simple use cases, Devs could do

<BarChart
	slotProps={{ 
		bar: { 
			onClick: (event, {dataIndex, seriesId, value}) => {...} 
		}
	}
/>

Note

For now I did not update the Pie chart interaction, but will have to be coherent with other charts API

Changelog

🚀 All charts have click handler. Test their respective docs demonstration to know more about the data format:

Big thanks to @giladappsforce and @yaredtsy for their contribution on exploring this feature

Breaking change

Pie charts onClick get renamed onItemClick for consistency with other charts click callback.

mui-bot · 2023-12-14T11:05:57Z

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

Updated pages:

Generated by 🚫 dangerJS against 2b4d53b

docs/data/charts/tooltip/tooltip.md

packages/x-charts/src/ChartsOnClickHandler/ChartsOnClickHandler.tsx

github-actions · 2024-01-10T15:49:00Z

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

github-actions · 2024-01-10T15:49:00Z

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

github-actions · 2024-01-24T10:14:42Z

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

docs/data/charts/lines/lines.md

docs/data/charts/bars/bars.md

Signed-off-by: Alexandre Fauquette <[email protected]>

github-actions · 2024-01-29T09:09:05Z

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

LukasTy

Superb work! 👍 👏 😍

Leaving some general nitpicks and raising a discussion point below.

Have you considered going with one event handler with a type prop to identify the type on the consumer side? 🤔
I've checked and it seems that both Nivo and Highcharts have only a single click handler. 🤷
At a glance it seems that we are adding quite a bit of complexity and it's a bit unclear if it is warranted. 🤔

docs/data/charts/bars/BarClickNoSnap.js

docs/data/charts/lines/LineClickNoSnap.js

LukasTy · 2024-01-29T14:47:19Z

docs/data/charts/pie/PieClickNoSnap.js

+      </Box>
+
+      <Stack direction="column" sx={{ width: { xs: '100%', md: '40%' } }}>
+        <Typography>click data</Typography>


Nitpick, but it feels like this section could use a bit extra "eye candy" to be a bit more clear and UX-friendly. 🤔
It also feels like these demos could use a reset button to clear the clicked data. 🤷
cc @noraleonte

Suggested change

<Typography>click data</Typography>

<Box sx={{display:'flex', justifyContent:'space-between'}}>

<Typography>Click on the chart!</Typography>

<IconButton aria-label="reset"> // reset button

<UndoOutlinedIcon/>

</IconButton>

</Box>

I agree, the reset button would make the experience a lot better 👌 We can do something simple, like this. And some slight improvement of the text

docs/data/charts/pie/pie.md

packages/x-charts/src/LineChart/MarkPlot.tsx

packages/x-charts/src/ScatterChart/Scatter.tsx

packages/x-charts/src/ScatterChart/ScatterChart.tsx

alexfauquette · 2024-01-30T09:59:54Z

Have you considered going with one event handler with a type prop to identify the type on the consumer side? 🤔

I made test for line chart:

For Recharts: they only add it to the container and behave a similar way to the onAxisClick I'm adding
For Nivo: The onClick works with voronoid but not with the vertical slices (you can only click on one point at the time)
For highchart, I did not found the docs about how the manage click 🙈

The complexity added by having multiple way of listening click event allows to adapt the chart behavior. For example modifying the pointer on hover if something is listening to the click.

I think the complexity is the largest for those components. Tree view, gauge, and other don't have those axes triggering issues, so they will only get itemClick

…-onclick

LukasTy · 2024-01-30T16:07:22Z

Gotcha. Seems fair. 👌

I think the complexity is the largest for those components. Tree view, gauge, and other don't have those axes triggering issues, so they will only get itemClick

Do you feel that it still makes sense to go for consistent naming (onItemClick vs onClick) even though we know that there will only be one kind of click handler? 🤔

noraleonte

Amazing work! 🍰 🎉

Leaving a small suggestion to follow up on Lukas' comment 👍

noraleonte · 2024-01-30T17:12:00Z

docs/data/charts/pie/PieClickNoSnap.js

+      </Box>
+
+      <Stack direction="column" sx={{ width: { xs: '100%', md: '40%' } }}>
+        <Typography>click data</Typography>


Suggested change

<Typography>click data</Typography>

<Box sx={{display:'flex', justifyContent:'space-between'}}>

<Typography>Click on the chart!</Typography>

<IconButton aria-label="reset"> // reset button

<UndoOutlinedIcon/>

</IconButton>

</Box>

I agree, the reset button would make the experience a lot better 👌 We can do something simple, like this. And some slight improvement of the text

noraleonte · 2024-01-30T17:26:12Z

docs/data/charts/pie/PieClickNoSnap.js

+          code={`// Data from item click
+${itemData ? JSON.stringify(itemData, null, 2) : '// click on the chart'}
+`}


Suggested change

code={`// Data from item click

${itemData ? JSON.stringify(itemData, null, 2) : '// click on the chart'}

`}

code={

itemData ? `// Data from item click:

${JSON.stringify(itemData, null, 2)}` : '// The data will appear here'

}

Maybe rephrasing these comments a bit could make the expected outcome a bit clearer 🤔

alexfauquette · 2024-01-30T18:27:38Z

Do you feel that it still makes sense to go for consistent naming (onItemClick vs onClick) even though we know that there will only be one kind of click handler? 🤔

It feel safer because we could have <PieChart /> getting onLegendClick, end other event handler

wascou · 2024-02-04T08:44:07Z

Hey thanks for the work, but one important thing is missing :)
Composability!
May you extend this to a nested ScatterPlot in a ResponsiveChartContainer?

LukasTy · 2024-02-05T08:31:20Z

@wascou have you checked this documentation section?

alexfauquette · 2024-02-05T10:08:12Z

Thanks @wascou for the heads up, I forgot to document how to use composition with scatter charts. The best documentation for now might be the code itself :)

mui-x/packages/x-charts/src/ScatterChart/ScatterChart.tsx

Lines 127 to 130 in 00b6b27

    
           <ChartsVoronoiHandler 
        
             voronoiMaxRadius={voronoiMaxRadius} 
        
             onItemClick={onItemClick as ChartsVoronoiHandlerProps['onItemClick']} 
        
           />

mui-x/packages/x-charts/src/ScatterChart/ScatterChart.tsx

Lines 141 to 145 in 00b6b27

    
           <ScatterPlot 
        
             slots={slots} 
        
             slotProps={slotProps} 
        
             onItemClick={disableVoronoi ? (onItemClick as ScatterPlotProps['onItemClick']) : undefined} 
        
           />

The <ResponsiveChartContainer /> shoudl have no impact on the click. And scatter plot has the same behavior as other. But I would recommend to use the click listener of the <ChartsVoronoiHandler />

Both are available only in v7

wascou · 2024-02-05T11:29:57Z

ah ok, no BC for this one ? I have to wait for a stable v7 then

wascou · 2024-02-21T12:19:08Z

Ok, works perfectly on my side. Voronoi i interresting but not very accurate when you have a lot of points in your scatter plot. I also put the onItemClick on ScatterPlot. Well done @alexfauquette and @LukasTy !

Signed-off-by: Alexandre Fauquette <[email protected]>

alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Dec 14, 2023

alexfauquette requested review from flaviendelangle and LukasTy January 8, 2024 13:56

flaviendelangle reviewed Jan 8, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024

alexfauquette mentioned this pull request Jan 10, 2024

[charts] Pass onClick to chart item #10549

Closed

4 tasks

alexfauquette force-pushed the add-onclick branch from ca12099 to 5ae198a Compare January 17, 2024 16:33

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2024

alexfauquette force-pushed the add-onclick branch from 5ae198a to e93cf2c Compare January 19, 2024 12:56

alexfauquette added 7 commits January 19, 2024 16:25

Add onAxisClickHandler

c605dbf

Support bar charts

08de3f1

support line chart

82adfa6

support scatter chart

2135216

pie breaking change

0b75ad4

docs update

90ad6d5

scripts

f09505f

alexfauquette force-pushed the add-onclick branch from e93cf2c to f09505f Compare January 19, 2024 15:29

alexfauquette marked this pull request as ready for review January 19, 2024 15:29

alexfauquette requested a review from flaviendelangle January 19, 2024 15:51

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2024

Merge remote-tracking branch 'upstream/next' into add-onclick

1452832

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2024

alexfauquette commented Jan 25, 2024

View reviewed changes

docs/data/charts/lines/lines.md Outdated Show resolved Hide resolved

alexfauquette commented Jan 25, 2024

View reviewed changes

docs/data/charts/bars/bars.md Outdated Show resolved Hide resolved

Apply suggestions from code review

60fb3a3

Signed-off-by: Alexandre Fauquette <[email protected]>

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2024

LukasTy reviewed Jan 29, 2024

View reviewed changes

alexfauquette added 3 commits January 30, 2024 12:59

carreful polishing from lukas

5906ded

Merge remote-tracking branch 'upstream/next' into add-onclick

77ac501

Merge branch 'add-onclick' of github.com:alexfauquette/mui-x into add…

315b477

…-onclick

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 30, 2024

alexfauquette added 3 commits January 30, 2024 13:19

fix

592090a

prettier

d145c74

scripts

e2eb2f7

LukasTy approved these changes Jan 30, 2024

View reviewed changes

noraleonte reviewed Jan 30, 2024

View reviewed changes

Nora improvmeent ideas

2b4d53b

alexfauquette merged commit 9d35151 into mui:next Feb 1, 2024
17 checks passed

This was referenced Feb 1, 2024

[charts] On line chart click #10575

Closed

[charts] Add support to event listeners (e.g. onClick) #10005

Closed

[chart] Make item interactives #10159

Closed

oliviertassinari added the new feature New feature or request label Feb 3, 2024

alexfauquette mentioned this pull request Feb 5, 2024

[charts][docs] Document click listener for scatter composition #11933

Open

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024

[charts] Add onClick support (mui#11411)

bca291d

Signed-off-by: Alexandre Fauquette <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Add `onClick` support #11411

[charts] Add `onClick` support #11411

alexfauquette commented Dec 14, 2023 •

edited

Loading

mui-bot commented Dec 14, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 29, 2024

LukasTy left a comment

LukasTy Jan 29, 2024

noraleonte Jan 30, 2024

alexfauquette commented Jan 30, 2024

LukasTy commented Jan 30, 2024

noraleonte left a comment

noraleonte Jan 30, 2024

noraleonte Jan 30, 2024

alexfauquette commented Jan 30, 2024

wascou commented Feb 4, 2024

LukasTy commented Feb 5, 2024

alexfauquette commented Feb 5, 2024

wascou commented Feb 5, 2024

wascou commented Feb 21, 2024

-        <Typography>click data</Typography>
+             <Box sx={{display:'flex', justifyContent:'space-between'}}>
+        <Typography>Click on the chart!</Typography>
+        <IconButton aria-label="reset">  // reset button
+          <UndoOutlinedIcon/>
+        </IconButton>
+      </Box>

[charts] Add onClick support #11411

[charts] Add onClick support #11411

Conversation

alexfauquette commented Dec 14, 2023 • edited Loading

Decision

Current solution

Why such a solution

Issues

Ideas

Idea 1

Idea 2

Note

Changelog

Breaking change

mui-bot commented Dec 14, 2023 • edited Loading

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 29, 2024

LukasTy left a comment

Choose a reason for hiding this comment

LukasTy Jan 29, 2024

Choose a reason for hiding this comment

noraleonte Jan 30, 2024

Choose a reason for hiding this comment

alexfauquette commented Jan 30, 2024

LukasTy commented Jan 30, 2024

noraleonte left a comment

Choose a reason for hiding this comment

noraleonte Jan 30, 2024

Choose a reason for hiding this comment

noraleonte Jan 30, 2024

Choose a reason for hiding this comment

alexfauquette commented Jan 30, 2024

wascou commented Feb 4, 2024

LukasTy commented Feb 5, 2024

alexfauquette commented Feb 5, 2024

wascou commented Feb 5, 2024

wascou commented Feb 21, 2024

[charts] Add `onClick` support #11411

[charts] Add `onClick` support #11411

alexfauquette commented Dec 14, 2023 •

edited

Loading

mui-bot commented Dec 14, 2023 •

edited

Loading