[charts] Allow setting a custom marker in legends and tooltips #16654

bernardobelchior · 2025-02-19T16:16:14Z

Related to #16640.

Allow setting a custom marker in legends and tooltips. This will be useful because scatter charts with custom shapes will probably want to have legends/tooltips with custom shapes.

Open questions:

Should we also allow configuring the label mark of piecewise and continuous legends? I don't think it makes sense for continuous legends, but it might for piecewise? I'm not familiar with them, so I'm not sure of the use case.

Left heatmap out in this PR because its tooltip implementation it separate. Will implement it in a follow-up PR, along with documentation.

mui-bot · 2025-02-19T16:20:35Z

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

Updated pages:

Generated by 🚫 dangerJS against 67e8283

packages/x-charts/src/ChartsLabel/ChartsLabelMark.tsx

github-actions · 2025-02-20T13:55:53Z

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

bernardobelchior · 2025-02-20T13:58:18Z

@alexfauquette @JCQuintas, a small question regarding API design. After I started working on adding custom shapes to scatter charts, I realized we'll probably want the scatter shape to match the legend/tooltip shape.
Do you have any suggestions on how to approach this?
In this PR I'm creating a labelMark slot for all charts that customizes the shape for legends and tooltips, but that doesn't solve the issue for custom scatter shapes. In this other PR I'm create a marker slot for scatter charts, but then a user that wants matching scatter shape and legend, they would need to provide two slots. What do you think of that? It's slightly more verbose, but maybe more flexible?

JCQuintas

Very clean implementation, nice work.

Left a comment of something that is probably important in some use-cases

JCQuintas · 2025-02-20T18:05:48Z

packages/x-charts/src/ChartsLabel/ChartsLabelMark.tsx

+  /**
+   * ID of the series this mark refers to.
+   */
+  seriesId?: SeriesId;


Probably needs dataIndex as well, since for PieCharts the entries are based on that 😢

Good point! I'll create a demo for pie charts, then 👍

Glad I tried to create a demo. It turns out the API didn't allow for the same customization in pie charts as it did for others 😄

packages/x-charts/src/ChartsTooltip/ChartsItemTooltipContent.tsx

…in a separate PR.

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

Co-authored-by: Alexandre Fauquette <[email protected]> Signed-off-by: Bernardo Belchior <[email protected]>

JCQuintas · 2025-02-21T10:27:38Z

packages/x-charts/src/ChartsLegend/legendContext.types.ts

+  /**
+   * The data index of the pie item
+   */
+  dataIndex?: number;


Sorry, in this case the itemId is already normalised to be id ?? dataIndex 😅

Yeah, but I find that a bit weird. If the user receives a id ?? dataIndex, how do they know if they should search for the ID or the data index? We don't enforce that all data points have an ID, so the user won't know whether to do data.find(d => d.id === itemId) or data[itemId].

Now that I think of it, dataIndex makes more sense because it's always present, while the ID is optional. In my opinion, we should always provide dataIndex for those reasons, but we can consider also providing itemId if you think it's useful.

What do you think?

So replace itemId with dataIndex? I could agree with that. I think I just didn't think critically about this specific part when making the id optional on pie items

Yeah, either the ID should be mandatory or we should use dataIndex. I'm not sure if the ID should be mandatory, though, so I guess dataIndex is probably the best option. What do you think?

We recently removed the requirement to have the id, it was required previously, so the correct way would be to use the dataIndex here. It is a breaking change though

alexfauquette

I'm not sure about the API.

Using the seriesId (and/or dataIndex) feels weird for real usecases. It forces users to take care of keeping in sync theirs slots and the data configuration.

From what I remember @JCQuintas The initial discussion was to allow either

Allowing to pass component like labelMark: 'line' | 'circle' | React.Elementto get{data: ...., labelMark: }` and is such case we render the component. Easy, without having to take care of slotsProps.

The other with slots is to allow labelMark extensions, and then it solves the pie chart issues by adding labelMark in the pie items.

alexfauquette · 2025-02-21T10:14:23Z

docs/data/charts/legend/LegendCustomLabelMark.js

+  { id: 0, data: [10], label: 'Series A' },
+  { id: 1, data: [15], label: 'Series B' },
+  { id: 2, data: [20], label: 'Series C' },
+  { id: 3, data: [10], label: 'Series D' },
+];


Suggested change

{ id: 0, data: [10], label: 'Series A' },

{ id: 1, data: [15], label: 'Series B' },

{ id: 2, data: [20], label: 'Series C' },

{ id: 3, data: [10], label: 'Series D' },

];

{ id: 0, data: [10, 15], label: 'Series A' },

{ id: 1, data: [15, 20], label: 'Series B' },

{ id: 2, data: [20, 25], label: 'Series C' },

{ id: 3, data: [10, 15], label: 'Series D' },

];

alexfauquette · 2025-02-21T10:16:40Z

docs/data/charts/legend/LegendCustomLabelMark.js

+  return (
+    <BarChart
+      series={seriesConfig}
+      xAxis={[{ scaleType: 'band', data: ['A'] }]}


Not directly related to the PR, but the axis "A" and series "Series A" bugs me (or to be precise let me think their is a bug in the tooltip

Suggested change

xAxis={[{ scaleType: 'band', data: ['A'] }]}

xAxis={[{ scaleType: 'band', data: ['cat. 1', 'cat. 2'] }]}

alexfauquette · 2025-02-21T10:22:09Z

docs/data/charts/legend/legend.md

+For more advanced use cases, you can also provide a component to the `labelMark` slot to fully customize the mark.
+
+{{"demo": "LegendCustomLabelMark.js" }}
+


Maybe also adding a h4 title "Built-in shapes" in the previous section.

It's mostly to simplify to visual processing of information. Titles are useful to split text and demos in coherent bloks.

For example here without reading I can't guess to which demos the paragraph refers to

Suggested change

For more advanced use cases, you can also provide a component to the `labelMark` slot to fully customize the mark.

{{"demo": "LegendCustomLabelMark.js" }}

#### Custom shapes

For more advanced use cases, you can also provide a component to the `labelMark` slot to fully customize the mark.

{{"demo": "LegendCustomLabelMark.js" }}

bernardobelchior · 2025-02-21T10:53:32Z

Using the seriesId (and/or dataIndex) feels weird for real usecases. It forces users to take care of keeping in sync theirs slots and the data configuration.

Yeah, that's a good point. The reason I did this was for consistency with the other PR for custom shapes where I'm also using slots. Maybe I should move away from slots in that example as well?

From what I remember @JCQuintas The initial discussion was to allow either

Allowing to pass component like labelMark: 'line' | 'circle' | React.Elementto get{data: ...., labelMark: }` and is such case we render the component. Easy, without having to take care of slotsProps.

Yeah, that API could work. The only downside I see is that if someone wants to use the same custom shape for all series, then they'd have to set the prop on every series, but I guess that's an acceptable trade-off.

The other with slots is to allow labelMark extensions, and then it solves the pie chart issues by adding labelMark in the pie items.

Sorry, I didn't get this option. What do you mean by "labelMark extensions"?

JCQuintas · 2025-02-21T11:29:59Z

My initial proposal for labelMarkType was to have something like the code below

interface LabelMarkTypeExtension {}

// Series config ...
labelMarkType?: 'square' | 'circle' | 'line' | keyof LabelMarkTypeExtension

// Custom Legend/Tooltip/Slot
const Legend = ({labelMarkType}) => {
  // decide what to do based on mark type.
}

I think the current implementation of slots would still be necessary, so it is a step towards that. And I would leave the labelMarkType changes to a different PR to not make it more confusing 😆

Providing seriedId|dataIndex might still be useful in some cases, but we can remove that to make the api cleaner.

Then on a future PR we implement the above, which would make the api more flexible from the series configuration side.

This way you would have one place to define the component if you want all of them to use the same thing, while if you want a more extensive approach you would have to use the slot+typeExtension+seriesConfig.

Accepting a labelMarkType?: ... | React.Element would be possible too, which would skip the typeExtension step, but I can't think about the advantages/drawbacks of each approach. It would probably make the slot useless like @alexfauquette mentioned, but at the cost of changing all the marks in one go. 🤷

alexfauquette · 2025-02-21T12:29:12Z

but at the cost of changing all the marks in one go. 🤷

Wy all the marks? The following would be still feasible. It only enforce on mark per series

{ id: 'line-1', labelMarkType: CustomMark1 },
{ id: 'line-2', labelMarkType: CustomMark2 },
{ id: 'line-1', labelMarkType: 'circle' },

bernardobelchior · 2025-02-21T12:59:04Z

So, what you're proposing is accepting a component in labelMarkType? Or should I also support slot?

bernardobelchior commented Feb 20, 2025

View reviewed changes

packages/x-charts/src/ChartsLabel/ChartsLabelMark.tsx Outdated Show resolved Hide resolved

bernardobelchior force-pushed the custom-shape-legend branch from 993a4d0 to e981343 Compare February 20, 2025 11:12

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

bernardobelchior force-pushed the custom-shape-legend branch from 111bfb4 to 0194616 Compare February 20, 2025 14:03

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

bernardobelchior requested review from JCQuintas and alexfauquette February 20, 2025 14:17

JCQuintas approved these changes Feb 20, 2025

View reviewed changes

bernardobelchior commented Feb 21, 2025

View reviewed changes

packages/x-charts/src/ChartsTooltip/ChartsItemTooltipContent.tsx Outdated Show resolved Hide resolved

bernardobelchior added 12 commits February 21, 2025 10:41

[charts] Allow setting a custom marker in legends

91ef197

Add docs

eb76428

Avoid spreading seriesId into DOM component

c6681a2

Fix types

b251241

Fix types

f06319f

Update docs. Rename var.

106132f

Revert style changes

26995eb

Add custom mark in tooltip

89b53ad

Fix cycle

d368e9f

Fix regression in tooltip. Remove heatmap changes as they'll be done …

8b22df6

…in a separate PR.

Add pie custom shape example.

f93758a

Fix lint

307ff39

bernardobelchior force-pushed the custom-shape-legend branch from 40c3cf9 to 307ff39 Compare February 21, 2025 09:41

Use dataIndex instead

48d4491

alexfauquette reviewed Feb 21, 2025

View reviewed changes

packages/x-charts/src/ChartsTooltip/ChartsTooltip.tsx Outdated Show resolved Hide resolved

Update packages/x-charts/src/ChartsTooltip/ChartsTooltip.tsx

67e8283

Co-authored-by: Alexandre Fauquette <[email protected]> Signed-off-by: Bernardo Belchior <[email protected]>

bernardobelchior marked this pull request as ready for review February 21, 2025 10:17

bernardobelchior requested a review from alexfauquette February 21, 2025 10:17

JCQuintas reviewed Feb 21, 2025

View reviewed changes

alexfauquette reviewed Feb 21, 2025

View reviewed changes

bernardobelchior requested a review from alexfauquette February 21, 2025 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Allow setting a custom marker in legends and tooltips #16654

[charts] Allow setting a custom marker in legends and tooltips #16654

bernardobelchior commented Feb 19, 2025 •

edited

Loading

mui-bot commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025

bernardobelchior commented Feb 20, 2025

JCQuintas left a comment

JCQuintas Feb 20, 2025

bernardobelchior Feb 21, 2025

bernardobelchior Feb 21, 2025

JCQuintas Feb 21, 2025

bernardobelchior Feb 21, 2025 •

edited

Loading

JCQuintas Feb 21, 2025

bernardobelchior Feb 21, 2025

JCQuintas Feb 21, 2025

alexfauquette left a comment

alexfauquette Feb 21, 2025

alexfauquette Feb 21, 2025

alexfauquette Feb 21, 2025

bernardobelchior commented Feb 21, 2025

JCQuintas commented Feb 21, 2025

alexfauquette commented Feb 21, 2025

bernardobelchior commented Feb 21, 2025

	xAxis={[{ scaleType: 'band', data: ['A'] }]}
	xAxis={[{ scaleType: 'band', data: ['cat. 1', 'cat. 2'] }]}

		For more advanced use cases, you can also provide a component to the `labelMark` slot to fully customize the mark.

		{{"demo": "LegendCustomLabelMark.js" }}

[charts] Allow setting a custom marker in legends and tooltips #16654

Are you sure you want to change the base?

[charts] Allow setting a custom marker in legends and tooltips #16654

Conversation

bernardobelchior commented Feb 19, 2025 • edited Loading

mui-bot commented Feb 19, 2025 • edited Loading

github-actions bot commented Feb 20, 2025

bernardobelchior commented Feb 20, 2025

JCQuintas 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

bernardobelchior Feb 21, 2025 • edited Loading

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

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

bernardobelchior commented Feb 21, 2025

JCQuintas commented Feb 21, 2025

alexfauquette commented Feb 21, 2025

bernardobelchior commented Feb 21, 2025

bernardobelchior commented Feb 19, 2025 •

edited

Loading

mui-bot commented Feb 19, 2025 •

edited

Loading

bernardobelchior Feb 21, 2025 •

edited

Loading