[charts] On line chart click

[giladappsforce]

on line chart click return the event, series data in clicked index, and the specific item clicked, with the index to extract data if needed.

Screen.Recording.2023-10-04.at.16.12.23.mov

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

[giladappsforce]

#10549

[mui-bot]

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

• Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

  [l10n] Improve Swedish (sv-SE) locale
  [l10n] Add Danish (da-DK) locale

• Update the documentation of supported locales by running yarn l10n
• Verify that you have added an export line in src/locales/index.ts for the new locale.
• Run yarn docs:api which should add your new translation to the list of exported interfaces.
• Clean files with yarn prettier.

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

Updated pages:

• docs/data/data-grid/server-side-data/aggregation.md
• docs/data/data-grid/server-side-data/index.md
• docs/data/data-grid/server-side-data/infinite-loading.md
• docs/data/data-grid/server-side-data/lazy-loading.md
• docs/data/data-grid/server-side-data/row-grouping.md

Generated by 🚫 dangerJS against 651e698

[oliviertassinari]

?

Suggested change

	onClick?.({ event, seriesData, itemData });
	onClick?.(event, seriesData, itemData);

[giladappsforce]

I thought it would be nicer to destructure anything you want but I removed it.

[alexfauquette]

What's a bit weird, is that the same onClick applies to both the root of the svg and the items. Usually in react we would have a onClick on the surface, and another one on items.

So I agree with you that destructuring makes sense here

Suggested change

	onClick?.({ event, seriesData, itemData });
	onClick?.(event, { seriesData, itemData });

Another solution would have been to introduce onClick and onSurfaceClick to be used as follows:

const LineChart = ({onClick , onSurfaceClick}) => {
   return <ChartsContainer onClick={onSurfaceClick}>
		{/* ... */}
		<LinePlot onClick={onClick}/>
	</ChartsContainer>
}

The onSurfaceClick would be transmitted to the container and the onClick would be more like the pie charts event listener.

This would allow to disable the axis listener when not needed:

 disableAxisListener={
-  tooltip?.trigger !== 'axis' && axisHighlight?.x === 'none' && axisHighlight?.y === 'none'
+  !onSurfaceClick && tooltip?.trigger !== 'axis' && axisHighlight?.x === 'none' && axisHighlight?.y === 'none'
 }

Another advantage is more flexibility when using composition since you could set onClick only on some sub-components.

But your current solution is elegant, and has the advantage of ensuring that highlight and click are in sync.

@giladappsforce What do you think?

[github-actions]

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

[alexfauquette]

Hey,

Sorry for the review delay, I was a bit confused because I had in mind something like Nivo: you can click on an item and you receive data about this item.

This PR is more Rechart like: you click and you get information about all the item on the line

The solution seems good. It would be interesting to add a demo like the pie charts to easily experiment how it deals when clicking on an item

[alexfauquette]

What would you think about moving this into the useAxisEvent with another useEffect such that when the charts does not require such event because disableAxisListener=true it just get skipped

React.useEffect(() => {
    const element = svgRef.current;
    if (element === null || disableAxisListener) {
      return () => {};
    }
    
    const handleOnClick => { ... }
    
    element.addEventListener('click', handleOnClick)

    return () => {    element.removeEventListener('click', handleOnClick) }

    }, [axis, item, series, svgRef, disableAxisListener, onClick])

If you want something that should handle line, bar and scatter at the same time, you can have a look at the axis tooltip that does a similar job of gathering information from different series

[alexfauquette]

I assume you plan to go with

onClick?: OnLineClick | OnBarClick | OnScatterClick;

I'm wondering if we coudl not do

onClick?: (event: React.MouseEvent<SVGElement>, params?: LineClickParams | BarClickParams | ScatterClickParams)

For PieChart it was not an issue since the onClick was passed to <PiePlot/> which is not a DOM element. It's a React.Fragment with multiple custom element. But here it's <Surface/> which correspond to the <svg /> which could get a native onClick

[github-actions]

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

[alexfauquette]

Closing since it's resolved by #11411 Sorry for the huge underestimation of the complexity of this feature. And thanks for this PR that helped me a lot to think about all the edge cases I did not had in mind 🙏