-
-
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 color scale #12490
[charts] Add color scale #12490
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
7bf37e8
to
f734ecb
Compare
91e0d49
to
29a9dcd
Compare
29a9dcd
to
f5d6720
Compare
docs/data/charts/styling/styling.md
Outdated
|
||
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) |
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.
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
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.
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?
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 there is an internal coherence I'm good, but yeah @samuelsycamore opinion will have more weight 👌
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.
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:
- Bar Charts provide two click handlers, or
- The Bar Chart (component) provides two click handlers
I moved the demos to |
docs/data/charts/styling/styling.md
Outdated
|
||
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) |
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.
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:
- Bar Charts provide two click handlers, or
- The Bar Chart (component) provides two click handlers
docs/data/charts/scatter/scatter.md
Outdated
|
||
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. |
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.
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. |
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.
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: |
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 does "by priority" mean here?
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 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.
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.
Nothing more to add, it's a massive improvement to the customizability of the charts 🥳
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.
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" }, |
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.
Nitpick: This is technically a BC, have you considered mentioning it in the release changelog? 🤔
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.
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
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.
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. |
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 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.
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.
I added those feedback in the followup PR
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.
Nice. 👍
thresholds: Value[]; | ||
/** | ||
* The colors used for each band defined by `thresholds`. | ||
* Should contain N+1 colors with N the number of thresholds. |
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 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. |
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.
Nitpick: Is this comment accurate? 🤔
WDYT, would mentioning the relation between values
and colors
make sense here? 🤔
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 mapChangelog
Breaking change
A tinny breaking change which is a typo fix.
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.