-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[charts] Add dataIndex
to series valueFormatter
#12745
[charts] Add dataIndex
to series valueFormatter
#12745
Conversation
Deploy preview: https://deploy-preview-12745--material-ui-x.netlify.app/ Updated pages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I left a few feedback
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
series: defaultizeValueFormatter(completedSeries, (v, _) => | ||
v == null ? '' : v.toLocaleString(), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like an experiment that has not been reversed 😉
Or there is a reason to add _
and not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I leave it out, typescript complains that the function signature is wrong. If I put it in but doesn't use it, eslint complains that it wasn't used. I left it cause I prefer having a unused error than a type error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about doing
- IFormatter extends (v: any, context: any) => string,
+ IFormatter extends (v?: any, context?: any) => string,
defaultizeValueFormatter
take as a second argument the default value formatter. But this function is not forced to use the two arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still produces a type error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind the type error is in the completedSeries
check the last commit, I made the function types more permissive. 11bf5a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution could be to define the type of the value formatter
--- a/packages/x-charts/src/models/seriesType/common.ts
+++ b/packages/x-charts/src/models/seriesType/common.ts
@@ -10,6 +10,11 @@ export type SeriesValueFormatterContext = {
dataIndex: number;
};
+export type SeriesValueFormatter<TValue> = (
+ value: TValue,
+ context: SeriesValueFormatterContext,
+) => string;
+
export type CommonSeriesType<TValue> = {
id?: SeriesId;
color?: string;
@@ -19,7 +24,7 @@ export type CommonSeriesType<TValue> = {
* @param {SeriesValueFormatterContext} context The rendering context of the value.
* @returns {string} The string to display.
*/
- valueFormatter?: <V extends TValue>(value: V, context: SeriesValueFormatterContext) => string;
+ valueFormatter?: SeriesValueFormatter<TValue>;
highlightScope?: Partial<HighlightScope>;
};
Then use it in the function
--- a/packages/x-charts/src/internals/defaultizeValueFormatter.ts
+++ b/packages/x-charts/src/internals/defaultizeValueFormatter.ts
@@ -1,17 +1,20 @@
-import { SeriesId } from '../models/seriesType/common';
+import { SeriesId, SeriesValueFormatter } from '../models/seriesType/common';
function defaultizeValueFormatter<
- IFormatter extends (v: any, context?: unknown) => string,
- ISeries extends { valueFormatter?: Function },
+ TValue,
+ ISeries extends { valueFormatter?: SeriesValueFormatter<TValue> },
>(
series: Record<SeriesId, ISeries>,
- defaultValueFormatter: IFormatter,
-): Record<SeriesId, ISeries & { valueFormatter: IFormatter }> {
- const defaultizedSeries: Record<SeriesId, ISeries & { valueFormatter: IFormatter }> = {};
+ defaultValueFormatter: SeriesValueFormatter<TValue>,
+): Record<SeriesId, ISeries & { valueFormatter: SeriesValueFormatter<TValue> }> {
+ const defaultizedSeries: Record<
+ SeriesId,
+ ISeries & { valueFormatter: SeriesValueFormatter<TValue> }
+ > = {};
Object.keys(series).forEach((seriesId) => {
defaultizedSeries[seriesId] = {
...series[seriesId],
- valueFormatter: (series[seriesId].valueFormatter as IFormatter) ?? defaultValueFormatter,
+ valueFormatter: series[seriesId].valueFormatter ?? defaultValueFormatter,
};
});
And we are getting a better TS because it fails the Formatting demo, noticing that we do not support the case where value is null
--- a/docs/data/charts/tooltip/Formatting.tsx
+++ b/docs/data/charts/tooltip/Formatting.tsx
@@ -89,7 +89,7 @@ export default function Formatting() {
xAxis={[{ data: years, scaleType: 'time', valueFormatter: yearFormatter }]}
series={lineChartsParams.series.map((serie) => ({
...serie,
- valueFormatter: currencyFormatter,
+ valueFormatter: (v) => (v === null ? '' : currencyFormatter(v)),
}))}
/>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better 😄
@@ -84,7 +84,10 @@ const formatter: Formatter<'line'> = (params, dataset) => { | |||
return { | |||
seriesOrder, | |||
stackingGroups, | |||
series: defaultizeValueFormatter(completedSeries, (v) => (v == null ? '' : v.toLocaleString())), | |||
// eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
This PR has been impacted by the merge of colorMap PR - ownerState={{ color }}
+ ownerState={{ color: getColor(dataIndex) ?? color }} |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
efc3523
to
61a65b6
Compare
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com> Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com> Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
SeriesValueFormatterContext
to all series value formatters (Line/Bar charts, Pie charts and Scatter charts)dataIndex
toSeriesValueFormatterContext
to allow users to access their own data add extra information to tooltipsresolves #11321