Skip to content
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 color scale #12490

Merged
merged 15 commits into from Apr 15, 2024
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Mar 19, 2024

Fix #11277
Fix #12378

The current strategy is to let axes get a colorMap attribute that define the mapping from values to color.

This configuration generate a colorScale: (value) => string thanks to some d3 functions. And some <lineargradient/> used to colorize the line/area charts.

Here are the docs demos to play with this new feature:

The following plan is to ad a notion of zAxis that would allow for example the scatter chart to use color as an additional indication. This z axis might also be used the same way by heat map

Changelog

Breaking change

A tinny breaking change which is a typo fix.

- ContinuouseScaleName
+ ContinuousScaleName

Charts now support the notion of color scale.
To do so, add a colorMap configuration to an axis, and the chart will use it to select colors.
Each impacted chart (bar charts, line charts, scatter charts) has a dedicated section explaining how this color map is impacting it.

scatter chart with gradient along y-axis

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Mar 19, 2024
@alexfauquette alexfauquette changed the title [charts] Add color scale [charts][WIP] Add color scale Mar 19, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 19, 2024
Copy link

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

@alexfauquette alexfauquette changed the base branch from next to master March 22, 2024 17:16
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 22, 2024
@alexfauquette alexfauquette changed the title [charts][WIP] Add color scale [charts] Add color scale Mar 28, 2024
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Show resolved Hide resolved
packages/x-charts/src/BarChart/BarPlot.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/BarChart/BarPlot.tsx Outdated Show resolved Hide resolved
docs/data/charts/lines/ColorScale.tsx Outdated Show resolved Hide resolved
docs/data/charts/bars/ColorScale.tsx Outdated Show resolved Hide resolved
docs/data/charts/scatter/ColorScale.tsx Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Show resolved Hide resolved
docs/data/charts/bars/bars.md Outdated Show resolved Hide resolved
docs/data/charts/bars/bars.md Outdated Show resolved Hide resolved

Learn more on how to use this feature with each chart component on their dedicated docs section:

- [bar chart](/x/react-charts/bars/#color-scale)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sometime use plural, sometime singular for for these (bar chart vs bar charts, same for line and scatter).
It would be nice to unify

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, I tend to use:

  • plural for charts in general (The bar charts have options for ...)
  • singular if describing the impact of a feature because I consider the chart the dev is working on "when clicking on the chart, ..."
Bar charts provides two click handlers:

- `onItemClick` for click on a specific bar.
- `onAxisClick` for a click anywhere in the chart

But could be simplified with only plural/singular @samuelsycamore do you have some insight on this point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an internal coherence I'm good, but yeah @samuelsycamore opinion will have more weight 👌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would say it's ok to vary singular and plural in this usage and there aren't really hard and fast rules. That said, I would probably lean more toward singular and capitalize them like proper nouns—the most important distinction here is between the Bar Chart component and the concept of bar charts in general, and the proper noun helps to make that more clear.

In the example you gave above @alexfauquette I would go with:

  1. Bar Charts provide two click handlers, or
  2. The Bar Chart (component) provides two click handlers

docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
@alexfauquette
Copy link
Member Author

I moved the demos to NoSnap because the snapshot faces an issue when using the <HighlighCode />

docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved

Learn more on how to use this feature with each chart component on their dedicated docs section:

- [bar chart](/x/react-charts/bars/#color-scale)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would say it's ok to vary singular and plural in this usage and there aren't really hard and fast rules. That said, I would probably lean more toward singular and capitalize them like proper nouns—the most important distinction here is between the Bar Chart component and the concept of bar charts in general, and the proper noun helps to make that more clear.

In the example you gave above @alexfauquette I would go with:

  1. Bar Charts provide two click handlers, or
  2. The Bar Chart (component) provides two click handlers

docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
docs/data/charts/scatter/scatter.md Outdated Show resolved Hide resolved

Like other charts, you can modify the [series color](/x/react-charts/styling/#colors) by using series color, or some color palette.

You can also modify color by using axes `colorMap` which maps values to colors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can also modify color by using axes `colorMap` which maps values to colors.
You can also modify the color of any given axis using `colorMap` which maps values to colors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formulation give me the feeling that the axes color will be impacted. But iit's more subtle. You provide a color instruction to the axis and it impact the series plot.

Like other charts, you can modify the [series color](/x/react-charts/styling/#colors) by using series color, or some color palette.

You can also modify color by using axes `colorMap` which maps values to colors.
The scatter charts use by priority:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does "by priority" mean here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the y-axis color is defined it's applied.
If the y-axis color is not defined, we look at the x-axis color, and if it's defined we apply it.
If the y-axis color is not defined, we apply the series color.

docs/data/charts/scatter/scatter.md Outdated Show resolved Hide resolved
docs/data/charts/styling/styling.md Outdated Show resolved Hide resolved
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing more to add, it's a massive improvement to the customizability of the charts 🥳

@alexfauquette alexfauquette merged commit 8efee23 into mui:master Apr 15, 2024
17 checks passed
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great customization improvement! 👏
The API looks nice and intuitive. 👍
Sorry for the late review, leaving a few total nitpicks. 😉

@@ -107,7 +107,7 @@
{ "name": "cheerfulFiestaPaletteDark", "kind": "Variable" },
{ "name": "cheerfulFiestaPaletteLight", "kind": "Variable" },
{ "name": "ComputedPieRadius", "kind": "Interface" },
{ "name": "ContinuouseScaleName", "kind": "TypeAlias" },
{ "name": "ContinuousScaleName", "kind": "TypeAlias" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: This is technically a BC, have you considered mentioning it in the release changelog? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also do something like export ContinuousScaleName as ContinuouseScaleName to avoid the breaking change, but it looks more problematic than the breaking change it tries to fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, to it also looks like a small BC that's not worth creating extra redundant technical debt for. 🙈

*/
max?: Value;
/**
* The colors to render. Can either be and array with the extrem colors, or an interpolation function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-The colors to render. Can either be and array with the extrem colors, or an interpolation function.
+The colors to render. It can be an array with the extremum colors, or an interpolation function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added those feedback in the followup PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. 👍

thresholds: Value[];
/**
* The colors used for each band defined by `thresholds`.
* Should contain N+1 colors with N the number of thresholds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Should contain N+1 colors with N the number of thresholds.
+Should contain N+1 colors, where N is the number of thresholds.

*/
values?: Value[];
/**
* The color palette.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Is this comment accurate? 🤔
WDYT, would mentioning the relation between values and colors make sense here? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Support color scale [charts] Allows to customize elements color based on its value
6 participants