[Lens] Chart level categorical color palettes #69800
Conversation
…workspace_panel_wrapper.scss Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
@cchaos I started work on providing a palette picker and laying some groundwork for coloring in Lens in general. Working on those some things popped up (the "Considerations" section). Could you have a look? Interested in your opinion. |
This should not have been the case. All charts should be using I've expressed this before, but my worry with using two different, yet same, color palettes for different chart types is going to hurt the user's connection of values when placed on a dashboard. There's been talk about evening the color palettes in all the charts in dashboards, how would you accomplish this with different palettes based on chart type? We can't use the lighter version for bar charts, or any other shape-based chart, because it doesn't provide enough contrast.
Do we really need to keep supporting this "palette"? I can understand Lens being able to support this when equalizing colors on a dashboard but do we have to make it an option in the select? If we must, can we call it "Legacy" not "Kibana Default" because "Default" is not longer true with this palette (at least not for Lens).
That shouldn't have ever been the case for bar charts, only area charts. Had a hard time tracking how far back that one goes but I'm 100% ok changing it for non-area charts.
I'll help with this when it's closer to done.
I'm guessing these were taken from Canvas, but EUI has a lovely set of hand-crafted palettes that compliment each other. Can we start with this set?
We also have an EuiColorPalettePicker component that can be used now.
WHAT are these two palettes? EUI only exports a single |
|
Kibana has an advanced setting
It's default value just has a mapping for "Count" with the "old" teal. Since Lens uses "Count of Records" as a label for the same, I think we should add that to the default value and give both the same color. I would suggest we're using the new color palette teal-like color for that. This would nevertheless change the color of all existing charts on "Count" slightly towards the new color palette. In my opinion the old teal and the new are close enough together to do that in a minor version, but would welcome other opinions on that. |
You are right, I confused something here, my bad. On master xy chart is currently using the regular
I'll go with option 1 for now, if it doesn't work out we can reconsider.
This sounds good to me, I will change the implementation |
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
Haven't tested this code yet, but here's my initial feedback:
|
|
@wylieconlon Thanks for your feedback I can definitely see your point here, and I don't have a strong feeling about removing it. However, I can still see these palettes being used the right way in some situations (e.g. when ordering terms alphabetically and this alphabetical order corresponds with a good<->bad scale) and besides that IMHO they are nice to look at also without explicit meaning. I slightly lean on leaving them out for now - removing them later on because we don't like the idea anymore is much harder than not introducing in the first place and gather feedback for a while @cchaos would that be OK for you? Agreed, that's also what I implemented. I wanted to highlight multiple possible approaches here, though. We can still introduce this using a stateful flag later on. I don't really see a big user benefit of exposing this as a setting - the difference is subtle and doesn't matter in a lot of cases (more than 10 colors is already an exotic case). What about just using a hue-shifted palette extension as a default? It's similar, I just wanted to show what can be done. It's simply reducing the lightness of each color by 20% Good point, I will look into that. Seems like something we should leave up the users, but I'm open to ideas how to make it easier to get to a good looking chart. I would like to avoid hiding options just because we think it won't look good though We could make current dark mode an input to the palette and change the colors. I will look into that as well. |
|
Changes:
|
|
All I'll ask is please don't make any changes/manipulate the palettes coming from EUI (unless it's to support more than 10 series/colors). If there needs to be a discussion about the palettes themselves, lets do that in EUI. I don't see the downside of giving users all options to colorize their chart. We can't know for sure what their data is representing or what trends they're trying to visualize. Eventually, we'll even support custom themes which would be even harder to try to manage color to data theory. We could give "suggestions" or hints, but if we have the palettes, lets let them use them. But I'll agree that removing them later is harder, so leaving them out for now is fine. |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/saved_objects_management/edit_saved_object·ts.saved objects management saved objects edition page allows to delete a saved objectStandard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Thanks for updating from my last feedback. I've now tested and reviewed the code, and here's my feedback about the behavior from most-to-least important:
-
I see why you've chosen to put each palette into a separate function so that they can define their arguments, but I think this goes too far- especially since we haven't built any stateful palettes yet. The decision tree I would use here is:
- Is it possible to just pass the paletteId in for the simple case, and palette state gets passed in via variable?
- Is it possible to pass the paletteID and palette state as JSON?
- Instead of defining a unique function for each, what's the fewest functions? I see three unique functions here: "round-robin categorical", "legacy" and "gradient".
-
The color lightening logic needs a second look
-
The forms to actually select a palette also need a second look
| }} | ||
| > | ||
| <EuiIcon type="gear" /> | ||
| </EuiButtonEmpty> |
There was a problem hiding this comment.
This is now inconsistent with the XY chart
| @@ -119,6 +120,7 @@ export function XyToolbar(props: VisualizationToolbarProps<State>) { | |||
| }} | |||
| anchorPosition="downRight" | |||
| > | |||
| <PalettePicker frame={props.frame} /> | |||
There was a problem hiding this comment.
I think this need an EuiFormRow with columnCompressed:
| })} | ||
| > | ||
| <ColorPicker {...props} /> | ||
| </EuiFormRow> |
There was a problem hiding this comment.
I think you've added this EuiFormRow to the wrong component...
| stroke-opacity: 1; | ||
| stroke-width: 0; | ||
| } | ||
|
|
||
| .series > path { | ||
| fill-opacity: .8; |
There was a problem hiding this comment.
This change doesn't belong in this PR, despite it being important. The main reason is that I think it needs to have a separate bullet point in the release notes.
| }} | ||
| datasourceMap={{ | ||
| testDatasource: mockDatasource, | ||
| testDatasource2: mockDatasource2, |
There was a problem hiding this comment.
| testDatasource2: mockDatasource2, |
| gray: { | ||
| title: i18n.translate('xpack.lens.palettes.grayLabel', { defaultMessage: 'gray' }), | ||
| ...buildRoundRobinCategorical('gray', euiPaletteGray), | ||
| }, |
There was a problem hiding this comment.
I would suggest flipping the order of positive and negative palettes here, since the order is kept in objects.
| return { | ||
| default: { | ||
| title: i18n.translate('xpack.lens.palettes.defaultPaletteLabel', { | ||
| defaultMessage: 'default', |
There was a problem hiding this comment.
Please use sentence case for these.
| @@ -97,6 +92,12 @@ export function PieComponent( | |||
| fillLabel.valueFormatter = () => ''; | |||
| } | |||
|
|
|||
| const totalSeriesCount = uniq( | |||
| firstTable.rows.map((row) => { | |||
| return columnGroups.map(({ col: { id: columnId } }) => row[columnId]).join(','); | |||
There was a problem hiding this comment.
There are a few edge cases here, and my preference is not to make changes to handle the edge cases but to at least note them in the code:
- Falsy values will all get the same color
- Dates will get different colors even if they are displayed the same as text
| /** | ||
| * A map of all available palettes (keys being the ids). | ||
| */ | ||
| availablePalettes: Record<string, PaletteDefinition>; |
There was a problem hiding this comment.
I know that objects inherently have sort order now, but an array instead of Record might make more sense here.
| @@ -251,6 +258,7 @@ export function XYChart({ | |||
| data.tables[layer.layerId].rows.some((row) => row[layer.xAccessor!] !== firstRowValue) | |||
| ) { | |||
| return false; | |||
| return false; | |||
There was a problem hiding this comment.
| return false; |
|
@wylieconlon I agree with almost all of your remarks and will fix them later this week or early next week, but I think we should discuss the first point:
I don't really see a downside of defining separate functions for each palette - to me this seems like the best approach among the options you listed. As long as we don't consider exposing the lens expression to the user, all of these approaches seem very similar to me - almost to a degree where refactoring it in the way you suggest isn't worth the effort. If we consider how a human user would use these, I definitely prefer the individual functions:
This is different than any other expression function so far and would introduce an implicit and easy to break interface. Where is it documented how this variable is called? Can we type check it? Provide help in form of suggestions etc. in the editor?
I know we do this in some places, but it's definitely not how the expression is meant to be used - also we can't leverage the type checking capabilities of the expression and it's super annoying to work with it
This is better, but still harder to use than separate functions for each palette - a user has to remember which "type" of palette they want to use before they can actually select it - having a separate function for each collapses these steps into one (without losing type-safety and auto-complete for providing arguments). The user would type My main point is that more functions don't hurt in any way, but other approaches do have downsides. I can think of one possible point here which is polluting the drop down of possible functions, but I don't really think this is a problem, because we are very generous with functions in other places as well to ensure type-safety - the editing experience can be fixed (e.g. only highlighting functions matching the current context type), the type safety with catch-all functions will be harder to work around. That said, I'm not at all convinced my opinion is the right one, it's just the conclusion I came to so far - very likely I forgot to consider something. @ppisljar What do you think about the topic? Short summary: Lens palettes can have different arguments (even if they don't have arguments at the moment). Do you think we should go with a scheme like this: or something like this More details in the rest of the comment. |
|
There are lots of constraints so handle these comments on face value:
|
|
Re the good comments by @cchaos
I agree with the layering(*) approach. The ease of use we give to the user to go with decent colors vs. shoot themselves in the foot color assignment wise matters a lot. For example, TSVB, by its prominent, in-line placement of the color picker option, and through the legacy dialog for color picking, initially feels like liberating freedom, but unintentionally promotes arbitrary data and background color picking with a high probability for deeply problematic color assignments. (*) There were actions (EUI dataviz docs and new EUI picker, @markov00's GAH presentation segments and earlier Product comments) about a layered approach where
It doesn't mean that giving the lowest level, therefore least informed/guided color configurability should be obscured and put in a counterintuitive place; it can be made directly accessible to the power user by eg. a keyboard shortcut, but there should be an access gradient that realizes our layered approach.
Nodding heavily, although mostly for the layering reason, rather than for the reason of palette existence. For example we really shouldn't offer those EUI palettes, if any, for dataviz, that haven't been designed or vetted for dataviz application. Maybe we could characterize them together if it's not done yet, they have various properties and dataviz applicability worthy of tabulating
++ for not rushing something to the user that may be a regret later |
|
I'm closing this PR as we want to change a bunch of stuff. Will open a new updated PR shortly. |
Fixes #68404
This PR lays the groundwork to use categorical color palettes in Lens.
TODOs:
euistandard palette for existing saved objects (to avoid migration)SeriesLayerbuilding in xy and pie color accessor functionsArchitecture
As colors are persisted when switching between visualizations, the state for these is not part of the visualization state but is managed by the frame itself. It's part of the root state reducer in
x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_management.ts. It is exposed to visualizations via the frame public api:Color functions are like indexpattern operations - they are statically defined, but all other components always interact with an abstract interface so it's fairly straightforward to make it a dynamic registry later on. It is stateful already (to sync colors with other charts on a dashboard when using the "Kibana" palette).
A color function looks like this:
As all of this is exposed via the frame, visualizations have full control over these (and could overwrite them theoretically). However in most cases it is enough to render the standard component to allow editing the palette
x-pack/plugins/lens/public/editor_frame_service/palettes/palette_picker.tsxExpression integration
The palette service owning the palette definitions in
x-pack/plugins/lens/public/editor_frame_service/paletteswill register an expression for each palette (lens_palette_eui,lens_palette_eui_pasteland so on) These functions take a serialized form of their internal state as argument (or nothing if there is no internal state) and return alens_palettedata structure which just holds a singlegetColorfunction which can be called by the renderer to determine the color. ThetoExpressionfunctions of the visualizations supporting the palette (xy and pie) use thetoExpressionfunction of the color function exposed on the frame api to put it as a separate arg of the render function on the expression:When the expression runs, the expression registered by the palette service will pass the right
getColorfunction to the renderer where it can be used.elastic-chartsintegrationWithin the renderer the
getColorfunction needs to get called with some information about the current series and its position in the hierarchy of series (also see #68330). In both pie and xy chart there is a "color accessor" function in which thegetColorfunction can be called. To do so the information available in there has to be mapped to the generic arguments ofgetColor. This is very different for xy and pie chart.User facing changes
Pie coloring changes
before
after
Respect color map in advanced settings in all palettes
If a color is specified for a term in the color map in advanced settings, it will be applied. There are multiple ways to do this (currently only relevant for pie charts - all examples set
CNtored):
* Pick up color map color in outer rings as well, but don't inherit this overwritten colors to child slices
* Pick up color map color in outer rings as well, and inherit this color to child-slicesI'm not sure which behavior is the best one.
Legacy color palette syncs with rest of dashboard
When the legacy color palette is used, colors for specific terms are synchronized across all visualizations - the first color assigned from the palette to a term is repeated if the term appears again.
Remove opacity from vislib bar charts
This was changed on accident - it's fixed on this PR so the colors are aligned with Lens visualizations with "legacy palette":
before
after
Palettes
Besides default and legacy palette, these are the supported options:
This is a deviation from the original list on the linked issue:
As the eui palette and eui colorblind palette are really close, I don't think we should offer both. Also, I don't think it makes much sense to offer two separate "Visualize" palettes with shared dashboard state and without - let's just share the state whenever possible.
Also, I didn't implement the "assign by name" overwrite palette because I think we have to consider this one separately (out of scope for this PR)
Palette settings
Having a custom state per palette is implemented, but I'm not sure whether we actually need it right now. We could offer a "reverse" checkbox for the gradient palettes, but this isn't helping much as the assignment is dynamic and pretty much random anyway. It will come in handy for manual overrides in the future.
A semi-useful feature to put the API to use right from the start is a setting to shift the color palette (like this):
Another option would be a "custom" palette in which the user chooses either a set of colors using the picker to build their own palette or the
EuiColorStopscomponent to define a range to pick from. For both of these I'm not sure whether a specific visualization is actually the right place to keep this state - I suggest we extend this functionality in a second iteration.