[charts] Add click listener to radar charts #18773
Conversation
|
Deploy preview: https://deploy-preview-18773--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: 🔺+11KB(+0.08%) - Total Gzip Change: 🔺+2.91KB(+0.07%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 🔺+1.53KB(+0.50%) gzip: 🔺+480B(+0.52%) |
CodSpeed Performance ReportMerging #18773 will not alter performanceComparing Summary
|
| onAreaClick={(event, d) => setItemData(d)} | ||
| onMarkClick={(event, d) => setItemData(d)} | ||
| onAxisClick={(event, d) => setAxisData(d)} |
There was a problem hiding this comment.
It is not clear for the user when a click happens in mark vs area
There was a problem hiding this comment.
What would you thin if I put a radio button to select between mark and area?
Because I assume devs will never use both at the same time
| - `onAreaClick` for click on a specific area. | ||
| - `onMarkClick` for click on a specific mark. | ||
| - `onAxisClick` for a click anywhere in the chart |
There was a problem hiding this comment.
What are the specific differences between them? Eg: A mark has access to the dataIndex, while area only has access to the series
There was a problem hiding this comment.
Yes, basically they provide all the information they have
There was a problem hiding this comment.
Now That I think about that the area could run some computation to know in which slice of the radar the pointer is and derive a dataIndex
There was a problem hiding this comment.
Could be an improvement, yeah
| onClick={(event) => onAreaClick?.(event, { type: 'radar', seriesId })} | ||
| cursor={onAreaClick ? 'pointer' : 'unset'} |
There was a problem hiding this comment.
Why we have onAreaClick here and in RadarSeriesArea? Same for the mark
There was a problem hiding this comment.
Because the RadarSeriesPlot renders both mark and area. So I need onAreaClick/onMarkClick.
Whereas RadarSeriesArea only render the area and so has an onItemClick
That's weird but users should not use at the same time RadarSeriesArea and RadarSeriesPlot. SO I try to make this feature available for whatever composition strategy devs pick
The renders all series together, such that the area of the second series is on top of the marks of the first one.
The and components make it possible to render all marks on top of all areas. You can also use them to render components between the areas and the marks.
There was a problem hiding this comment.
Why have both then? 🤔
Both what?
There was a problem hiding this comment.
RadarSeriesPlot and separate RadarSeriesArea? We seem to only use RadarSeriesArea in our code
There was a problem hiding this comment.
Yes, but we export both such that devs who want the other approach get it out of the box.
The question is "If series A area is on top of series B". Should the series B mark be on top of series A or bellow.
We ended up deciding that, depending on how you want to interact both can make sense, and then we pick one solution for the single component chart but export components for both usecases
To redo RadarSeriesPlot you would have to do the following
RadarSeriesPlot => <g>
<RadarSeriesArea seriesId={id1} />
<RadarSeriesMark seriesId={id1} />
<RadarSeriesArea seriesId={id2} />
<RadarSeriesMark seriesId={id2} />
<RadarSeriesArea seriesId={id3} />
<RadarSeriesMark seriesId={id3} />
</g>There was a problem hiding this comment.
Ah, so it is a component to help users who want to have this different order?
There was a problem hiding this comment.
Yes, exactly. The docs can probably be improved if you did not get it
| // The series to axis mapping is not yet-needed | ||
|
|
||
| // const providedRotationAxisId = seriesItem.rotationAxisId; | ||
| // const providedRadiusAxisId = seriesItem.radiusAxisId; | ||
|
|
||
| // const axisKey = isRotationAxis ? providedXAxisId : providedYAxisId; | ||
| // if (axisKey === undefined || axisKey === USED_AXIS_ID) { |
There was a problem hiding this comment.
Does this comment provide any future value?
There was a problem hiding this comment.
Technically uncommenting them should maket the feature work for a polar bar/line chart. But I can remove if you find it too noisiy
There was a problem hiding this comment.
we can probably remove it for now unless you are working on that feature already 😆
Fix #18750
The implementation is an adaptation of the cartesian
onAxisClickplus the line chart behavior withonItemClickandonLineClick.