-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Add series color callback #20084
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 series color callback #20084
Conversation
|
Deploy preview: https://deploy-preview-20084--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
| xScale: D3Scale; | ||
| yScale: D3Scale; | ||
| /** | ||
| * @deprecated Use `colorGetter` instead. |
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.
The color can be obtained by calling colorGetter(), so this color is redundant. That was my logic for deprecating this. Let me know what you think.
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 feels a bit weird because all the series now have color that support a callback, and scatter get its color deprecated in favor of the colorGetter
CodSpeed Performance ReportMerging #20084 will not alter performanceComparing Summary
|
e5711c8 to
61ed9e5
Compare
| {seriesOrder.map((seriesId) => { | ||
| const { id, xAxisId, yAxisId, zAxisId, color } = series[seriesId]; | ||
|
|
||
| // FIXME: V9: remove this cast as it will no longer be necessary |
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.
Added an action item in tasks for v9 to remove these fixmes
a06ce47 to
e3de132
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
f177611 to
43f4e87
Compare
| xScale: D3Scale; | ||
| yScale: D3Scale; | ||
| /** | ||
| * @deprecated Use `colorGetter` instead. |
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 feels a bit weird because all the series now have color that support a callback, and scatter get its color deprecated in favor of the colorGetter
| color={ | ||
| typeof series.color === 'function' | ||
| ? series.color({ value: min.value, dataIndex: min.index }) | ||
| : series.color | ||
| } |
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.
From a user perspective this feel paintfull. 99% of the time color is just a string. Having to do edge case for the function option is not optimal.
What about getting the colorGetter and at the same place we defaultise the color according to the colorPalette, we add a logic saying "if colorGetter is deined, I do color = colorGetter(null)"
That way people that don't care about callback can ignore this option. and those who care can simply do color={series.collorGetter?.(value) ?? series.color}
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.
From a user perspective this feel paintfull
Yeah, I kind of agree. We can expose colorGetter and color is the result of colorGetter(null), i.e., a default color for the series.
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.
So what you're saying is that a series will accept a colorGetter prop that has priority over color, right?
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.
So what you're saying is that a
serieswill accept acolorGetterprop that has priority over color, right?
Yes, at least that what I though while reading the PR. Maybe there is a better tradeof
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.
Updated, let me know what you think 😄
ea1d740 to
5dc7e1b
Compare
| series: [ | ||
| { | ||
| data: netSpendInPounds, | ||
| colorGetter: (data) => clubColors[data.dataIndex], |
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.
Should we call it getColor instead of colorGetter? Or getDataPointColor?
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.
colorGetter has the advantage that when you search for color you find it just after in the API page or the autocomplet. But I agree getColor feels more natural
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.
Yeah, that was my conclusion as well.
@JCQuintas any input? If not, I'd rather stick with colorGetter as I value the discoverability more
| const { seriesId, points, color, hideMark, fillArea } = series; | ||
| const getColor = getSeriesColorFn(series); |
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.
Should we put that in the useRadarSeriesData() to avoid having some computation in the hook and others on the component
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.
Sure
| series: [ | ||
| { | ||
| data: netSpendInPounds, | ||
| colorGetter: (data) => clubColors[data.dataIndex], |
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.
colorGetter has the advantage that when you search for color you find it just after in the API page or the autocomplet. But I agree getColor feels more natural
509d888 to
808f940
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
808f940 to
7173e06
Compare
7173e06 to
1ba3a9f
Compare
JCQuintas
left a comment
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.
Shouldn't this also extend to pro packages?
Co-authored-by: Jose C Quintas Jr <[email protected]> Signed-off-by: Bernardo Belchior <[email protected]>
Do you mean that I forgot to update the pro series types? Yeah, I did 😅 on it |
|
Actually, all pro charts omit the |
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ui#19851) Signed-off-by: Michel Engelen <[email protected]> Signed-off-by: Gene Arch <[email protected]> Co-authored-by: Michel Engelen <[email protected]> Co-authored-by: Olivier Tassinari <[email protected]> Co-authored-by: Gene Arch <[email protected]> Co-authored-by: Rom Grk <[email protected]>
…ctionsCellItem) (mui#19513) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: michelengelen <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
But shouldn't it be implemented for them? Like, the funnel chart has a similar color setting as the pie chart. The sankey is completely custom, but it could, technically, benefit from a We can do it in a followup though if you agree |
|
Yeah, I'll do that as a follow-up 👍 |
Add series
colorGetterprop with a(data: { value: TValue, dataIndex: number}) => stringsignature.Demo: