mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
3.79k stars 1.12k forks source link

[charts] Allow `series.label` property to receive a function with the "location" it is going to be displayed on #12830

Open JCQuintas opened 2 weeks ago

JCQuintas commented 2 weeks ago

resolves #12482

Todo:

JCQuintas commented 2 weeks ago

@alexfauquette what do you think about having a labelFormatter on the series in order to solve https://github.com/mui/mui-x/issues/12482?

https://codesandbox.io/p/sandbox/12482-labelformatter-dj3d9c?file=%2Fsrc%2Fdemo.tsx%3A8%2C32

mui-bot commented 2 weeks ago

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

Updated pages:

Generated by :no_entry_sign: dangerJS against 590056787b4a7a996e71328b56347dc94584b6d8

alexfauquette commented 2 weeks ago

what do you think about having a labelFormatter on the series in order to solve https://github.com/mui/mui-x/issues/12482?

IMHO the formatter is useful to define a rendering once and use it multiple times.

For example with valueFormatter, you define it once for the series, and it's applied to all the values.

Since series have one label that can be displayed in the tooltip or the legend, you end up with only two cases.

From a DX point of view, this syntax seems easier to read and document:

label: 'Duration',
- labelFormatter: (v, { location }) => location === 'tooltip' ? `${v} (HH:MM:SS)` : v,
+ labelTooltip: 'Duration (HH:MM:SS)",
JCQuintas commented 2 weeks ago

From a DX point of view, this syntax seems easier to read and document:

I kind of agree, but we have Pie charts which would mean we need to add labelTooltip to the data as well.

So pretty much series.labelTooltip and when type=pie series[].data[].labelTooltip. If that seems better, should I also change arcLabel to accept 'labelTooltip'?

alexfauquette commented 2 weeks ago

I kind of agree, but we have Pie charts which would mean we need to add labelTooltip to the data as well.

Always a pain to support pie charts :)

What about having tooltipLabel and legendLabel (on the same idea as arcLabel).

It would be two formatter with type ((item: DefaultizedPieValueType) => string). For those two new ones, we don't have to add 'formattedValue' | 'label' | 'value' because only 'label' makes sense for the tooltip/legend.

JCQuintas commented 2 weeks ago

Yeah, Pie charts are also odd that we use their dataset as "series" and that messes with my feelings a lot 😛

JCQuintas commented 1 week ago

So thought a bit more about this, and I don't see a lot of value in having all these different formatters when one with a context would do 🤔

This PR implements

series[].label: string
series[].labelFormatter: ((context: SeriesLabelFormatterContext) => string)

series[type=pie].data[].label = string
series[type=pie].data[].arcLabel = 'formattedValue' | 'label' | 'value'| ((item: DefaultizedPieValueType) => string)

Your suggestion would be

series[].legend = 'string'
series[].tooltipLabel = (item: DefaultizedPieValueType) => string
series[].legendLabel = (item: DefaultizedPieValueType) => string

series[type=pie].data[].label = 'string'
series[type=pie].data[].arcLabel = 'formattedValue' | 'label' | 'value'| ((item: DefaultizedPieValueType) => string)
series[type=pie].data[].tooltipLabel = (item: DefaultizedPieValueType) => string
series[type=pie].data[].legendLabel = (item: DefaultizedPieValueType) => string

Is this correct?

I think we could build on the first case to make it smaller, like, if formatting the label is important, rather than having label be a a string, we could also allow functions

series[].label: string | ((context: SeriesLabelFormatterContext) => string);

series[type=pie].data[].label = string | ((context: SeriesLabelFormatterContext) => string)
series[type=pie].data[].arcLabel = 'formattedValue' | 'label' | 'value'| ((item: DefaultizedPieValueType) => string)

Would this be a good approach?

alexfauquette commented 1 week ago

Would this be a good approach?

Yes, that looks even better :+1:

What about replacing the context with just the location. and we will add the context if necessary latter as a second argument:

- series[].label: string | ((context: SeriesLabelFormatterContext) => string);
+ series[].label: string | (location: "tooltip" | "axis") => string;
JCQuintas commented 1 week ago

What about replacing the context with just the location. and we will add the context if necessary latter as a second argument:

Sure that can work

github-actions[bot] commented 3 days ago

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