[Lens] Categorical color palettes #75309

flash1293 · 2020-08-18T15:48:50Z

This PR implements a general palette service in the charts plugin an exposes it in Lens to be able to pick palettes there.

This PR roughly contains three separate pieces which are linked to each other but can be reviewed separately:

Canvas changes

Move palette definition to the charts plugin
Change the shape of the palette expression data type to

{
  type: 'palette';
  name: string;
  params: unknown;
}

Move color expansion (getColorsFromPalette) in plot and pie functions into the charts plugin (part of the "custom" plugin)
Move plot and pie functions into public plugin so they can depend on the public contract of the charts plugin to pull in palette logic
Pass palettes into embeddable renderer to rewrite expression including a custom palette
Lens chart will use the palette configured in Lens if not overwritten

Charts changes

Add expressions functions palette and system_palette - both produce a palette data type (palette is a shortcut function for the "custom" palette used in canvas):

{
  type: 'palette';
  name: string;
  params: unknown;
}

Add palette registry which can be lazily loaded using charts.palettes.getPalettes()
Each palette is defined by a PaletteDefinition, featuring functions for serializing it to the expression, a getter for an array of colors and a getter for an individual colors which gets a configuration passed in
Supported palettes:
- default: EUI palette, differentiating between behind text and regular palette - assigned by position of the current series
- kibana_palette: Using the vislib color assignment by series name only (will sync up with vislib charts)
- warm/cool/positive/negative/grey/status/temperature/complimentary: Assigned by position of the current series, using full spectrum of the palette
- custom: Using former canvas palette logic based on chrome-js or round-robin assignment

Lens changes

Pass down palette registry as part of frame API as availablePalettes
xy and pie serialize palettes in their state and put them onto the expression as well (with a fallback to a set theme variable)
Visualization API is extended by getMainPalette to return current main palette based on state, and an optional parameter on initialize and getSuggestions to pass it into another chart type - this is the way to hand over a palette from one chart to another one. A visualization can use multiple palettes internally
Xy chart allows to define a palette as part of the break down dimension per layer. The first layer palette is transferred to other chart types. If the break down dimension is deleted, it falls back to the default palette
Pie chart allows a single palette which is editable from within each slice dimension
No indicator in dimension trigger yet (this will be handled by a separate PR)

…workspace_panel_wrapper.scss Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

…toolbar

wylieconlon

I tested this in Chrome and reviewed the code. I have left a few comments about the code, and I found one bug that I couldn't identify the cause of. The bug is:

Add any saved Lens vis to canvas
Edit the expression by adding an invalid palette, something like palette={}. This will cause an error state Can not cast null to palette
Delete the invalid palette and the visualization won't update, the console shows

Embeddable has been destroyed
    at Embeddable.updateInput (embeddable.tsx:134)
    ```

wylieconlon · 2020-10-29T22:23:25Z

src/plugins/charts/common/palette.ts

+  '#F1932D',
+  '#E8601C',
+  '#DC050C',
+];


This choice of palette for the fallback doesn't make a lot of sense to me- why not default to EUI, or throw an error when missing a palette definition?

I worry that we could get stuck with a palette that can never be changed, similar to the 7-color default vislib palette. Also, the name of the palette here is Paul Tol 14

This is done for BWC with Canvas. The palette function is only used by canvas and is already in use there (with these defaults). We can think about changing this with 8.0

I see, can you add a code comment about this?

src/plugins/charts/common/palette.ts

wylieconlon · 2020-10-29T22:25:02Z

src/plugins/charts/common/palette.ts

+      params: {
+        help: i18n.translate('charts.functions.systemPalette.args.paramsHelpText', {
+          defaultMessage: 'Palette specific params of the palette',
+        }),


I don't understand what this is supposed to contain- or even what the datatype is. Do we need this?

It's not used right now, but this is the place system palettes would use that to serialize custom params. E.g.
{system_palette id="myFancyPalette" params={fancy_palette_params somethingCustom=true }}. We can also remove it for now if you think that would be better.

My preference would be to remove it until needed.

src/plugins/charts/common/palette.test.ts

wylieconlon · 2020-10-29T22:33:23Z

src/plugins/charts/public/services/palettes/palettes.tsx

+import { LegacyColorsService } from '../legacy_colors';
+
+function buildRoundRobinCategoricalWithMappedColors(): Omit<PaletteDefinition, 'title'> {
+  const colors = euiPaletteColorBlind({ rotations: 2 });


I agree that increasing the number of EUI colors from 10 to 20 is helpful for medium-size charts, but have we showed this to others to confirm?

I discussed this approach with @AlonaNadler

x-pack/plugins/canvas/canvas_plugin_src/functions/external/saved_lens.ts

…colors

flash1293 · 2020-10-30T09:43:57Z

@wylieconlon Thanks for the review - I either fixed your comments in code are answered to the comments. I checked the bug but it doesn't seem like this is caused by my changes. I did the same with the visualize embeddable and it causes the same problem there (embeddable destroyed). I think it makes sense to open a separate bug for this.

wylieconlon

Thanks for the updates, did not test again but LGTM

wylieconlon · 2020-10-30T20:59:48Z

src/plugins/charts/common/palette.ts

+  '#F1932D',
+  '#E8601C',
+  '#DC050C',
+];


I see, can you add a code comment about this?

wylieconlon · 2020-10-30T21:00:10Z

src/plugins/charts/common/palette.ts

+      params: {
+        help: i18n.translate('charts.functions.systemPalette.args.paramsHelpText', {
+          defaultMessage: 'Palette specific params of the palette',
+        }),


My preference would be to remove it until needed.

MichaelMarcialis

Hey, @flash1293. This looks great! I left one small comment below to use the compressed form style for the color palette picker.

Otherwise, I just had one question regarding adding Lens visualizations to Canvas workpads. I noticed that all visualizations added to Canvas workpads had transparent backgrounds by default, except for Lens visualizations (which had a white background). Is this intentional? Not sure if part of this PR, but thought I'd ask.

MichaelMarcialis · 2020-10-30T20:54:18Z

x-pack/plugins/lens/public/shared_components/palette_picker.tsx

+      })}
+    >
+      <>
+        <EuiColorPalettePicker


EuiColorPalettePicker requires a prop of compressed in order to be displayed in the same compressed style as the rest of the form.

flash1293 · 2020-11-02T08:06:14Z

Otherwise, I just had one question regarding adding Lens visualizations to Canvas workpads. I noticed that all visualizations added to Canvas workpads had transparent backgrounds by default, except for Lens visualizations (which had a white background). Is this intentional? Not sure if part of this PR, but thought I'd ask.

@MichaelMarcialis Good point. It's not a side effect of this PR, but definitely something we should fix. Could you open a separate issue?

…colors

clintandrewhall

I have some feedback, but approving to unblock.

clintandrewhall · 2020-11-02T17:24:19Z

src/plugins/charts/common/palette.ts

+  name: string;
+  params?: T;
+}
+export const defaultCustomColors = [


Suggested change

export const defaultCustomColors = [

// This set of defaults originated in Canvas, which, at present, is the primary

// consumer of this function. Changing this default requires a change in Canvas

// logic, which would likely be a breaking change in 7.x.

export const defaultCustomColors = [

Ah, forgot about that, thanks for the great suggestion!

src/plugins/charts/common/palette.ts

clintandrewhall · 2020-11-02T17:30:52Z

x-pack/plugins/canvas/i18n/functions/dict/pie.ts

 import { FunctionHelp } from '../function_help';
 import { FunctionFactory } from '../../../types';
 import { Legend } from '../../../types';
 import { CSS, FONT_FAMILY, FONT_WEIGHT, BOOLEAN_FALSE } from '../../constants';

-export const help: FunctionHelp<FunctionFactory<typeof pie>> = {
+export const help: FunctionHelp<FunctionFactory<ReturnType<typeof pieFunctionFactory>>> = {


nit: this feels very dense... might be nice to shorten this to a common type? Probably beyond the scope of this PR, though.

Good point, I added FunctionFactoryHelp to function_help.ts

clintandrewhall · 2020-11-02T17:34:06Z

x-pack/plugins/canvas/public/functions/pie.ts

+}
+
+export function pieFunctionFactory(
+  initialize: InitializeArguments


I think it makes more sense to pass the Palette Service to this function, rather than InitializeArguments. The contract is much more explicit.

AlonaNadler · 2020-11-02T23:13:51Z

Hey looks awesome!!! it works very nicely 👍
I created a really nice dashboard playing with colors

few questions:

why can't I change the color in the treemap but if I changed it in the pie chart it persists in the treemap?
I saw that in 7.10 BC as well, something is wrong in the area stacked colors, the white areas it is unclear what to assume from them
It's probably a different PR but in the mocks, the color should be visible in the configurator without clicking on it. The reason it is important is since in some chart you configure from the breakdown by in others from the dimensions and it might be confusing, the way to solve it is to show visibly the colors palette in the dimension it can be configured
Im not sure I understand how color applies on the external circle in a pie/donut chart. Should it be based on the internal ring color? should colors be by the value? so the same values have the same color? I'm ok with it as is for 7.11 but just wondering

…colors

flash1293 · 2020-11-03T11:03:00Z

Thanks for the feedback!

@AlonaNadler answers to your questions:

why can't I change the color in the treemap but if I changed it in the pie chart it persists in the treemap?

Great catch, that's a bug. Should be fixed now.

I saw that in 7.10 BC as well, something is wrong in the area stacked colors, the white areas it is unclear what to assume from them

I think that's caused by series not being available on all buckets. It's not caused by this PR, could you open an elastic-charts issue for this?

It's probably a different PR but in the mocks, the color should be visible in the configurator without clicking on it. The reason it is important is since in some chart you configure from the breakdown by in others from the dimensions and it might be confusing, the way to solve it is to show visibly the colors palette in the dimension it can be configured

Yes, working on that separately. It will also be part of 7.11

Im not sure I understand how color applies on the external circle in a pie/donut chart. Should it be based on the internal ring color? should colors be by the value? so the same values have the same color? I'm ok with it as is for 7.11 but just wondering

The color is assigned based on the inner ring, then lightened up for the outer rings (like it's working right now, didn't change that behavior). Once we implement color sync for Lens (#81976), we can decide whether we want to color by value on the outer layers as well like visualize is doing it today.

mbondyra · 2020-11-03T16:01:36Z

LGTM. Tested in Safari, it's working really well!

flash1293 · 2020-11-04T08:23:47Z

@elasticmachine merge upstream

kibanamachine · 2020-11-04T10:14:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0d20d56

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`canvas`	1119	1116	-3
`charts`	39	49	+10
`lens`	565	558	-7
total			-0

async chunk count

id	before	after	diff
`charts`	0	1	+1

async chunks size

id	before	after	diff
`canvas`	1.3MB	1.3MB	+9.3KB
`charts`	0.0B	57.4KB	+57.4KB
`lens`	1.0MB	1020.4KB	-26.7KB
`visTypeTimeseries`	1.7MB	1.7MB	+6.0B
`visTypeVislib`	702.0KB	702.0KB	+12.0B
total			+40.0KB

distributable file count

id	before	after	diff
`default`	42713	42709	-4
`oss`	22410	22415	+5

page load bundle size

id	before	after	diff
`canvas`	919.3KB	908.8KB	-10.5KB
`charts`	134.9KB	147.9KB	+13.0KB
`lens`	49.5KB	49.7KB	+292.0B
`visTypeTagcloud`	19.5KB	19.6KB	+6.0B
total			+2.8KB

History

💚 Build #85478 succeeded 6926a6c
💚 Build #85150 succeeded 7c085ed
💚 Build #85052 succeeded df33fa2
💚 Build #84778 succeeded 49f2294
💚 Build #84712 succeeded e4fd331

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

flash1293 and others added 30 commits June 16, 2020 15:22

add toolbar api

bebac29

fix teests

e983014

fix types

3fbff62

fix functional test

426f2ce

Merge remote-tracking branch 'upstream/master' into lens/toolbar

44a17c3

add back title

8c7381e

remove separator line

64ee8ef

Merge remote-tracking branch 'upstream/master' into lens/toolbar

9bb7843

fix i18n

c1ddaaa

Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/_…

e51ff5e

…workspace_panel_wrapper.scss Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into lens/toolbar

e3c6f94

Merge branch 'lens/toolbar' of github.com:flash1293/kibana into lens/…

a278772

…toolbar

Merge remote-tracking branch 'upstream/master' into lens/toolbar

bfb6898

Merge remote-tracking branch 'upstream/master' into lens/toolbar

b3fb6e8

do not show unsaved when there is no cahrt

2434486

revert unnecessary changes

85c128e

Merge remote-tracking branch 'upstream/master' into lens/toolbar

1ece895

always show title if available

34cff58

start implementing color picker fundamentals

59ac920

implement palette service

51ac271

Merge remote-tracking branch 'upstream/master' into lens/color-palettes

57cfda7

Merge remote-tracking branch 'upstream/master' into lens/color-palettes

f65b9fb

keep working on color palettes

35b421e

Merge remote-tracking branch 'upstream/master' into lens/color-palettes

e152365

fix some things

01d1cd8

Merge remote-tracking branch 'upstream/master' into lens/color-palettes

bf81263

fix tests

c3cab97

render single layer tree maps correctly

0082554

Merge remote-tracking branch 'upstream/master' into lens/color-palettes

99266bc

fix tests

2094bb4

wylieconlon suggested changes Oct 29, 2020

View reviewed changes

flash1293 added 3 commits October 30, 2020 09:54

Merge remote-tracking branch 'upstream/master' into lens/categorical-…

382f71e

…colors

review comments

c2de739

review comments

49f2294

flash1293 requested a review from wylieconlon October 30, 2020 09:44

wylieconlon approved these changes Oct 30, 2020

View reviewed changes

MichaelMarcialis reviewed Oct 30, 2020

View reviewed changes

flash1293 added 3 commits November 2, 2020 09:07

Merge remote-tracking branch 'upstream/master' into lens/categorical-…

2575403

…colors

review comments

df33fa2

refactor color assignment

7c085ed

clintandrewhall approved these changes Nov 2, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lens/categorical-…

79065b4

…colors

review comments

6926a6c

mbondyra approved these changes Nov 3, 2020

View reviewed changes

nickofthyme mentioned this pull request Nov 3, 2020

Refactor vis_type_xy plugin to use palette service #82495

Closed

Merge branch 'master' into lens/categorical-colors

0d20d56

flash1293 merged commit fe3b053 into elastic:master Nov 4, 2020

flash1293 added a commit to flash1293/kibana that referenced this pull request Nov 4, 2020

[Lens] Categorical color palettes (elastic#75309)

ba3a82a

flash1293 mentioned this pull request Nov 4, 2020

[7.x] [Lens] Categorical color palettes (#75309) #82586

Merged

flash1293 added a commit that referenced this pull request Nov 4, 2020

[Lens] Categorical color palettes (#75309) (#82586)

0452f44

This was referenced Nov 5, 2020

[Lens] Categorical color functions #68330

Closed

[Lens] break down by - allow users to select their color palette #53661

Closed

palette context type alignment #70943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Categorical color palettes #75309

[Lens] Categorical color palettes #75309

flash1293 commented Aug 18, 2020 •

edited

Loading

wylieconlon left a comment

wylieconlon Oct 29, 2020

flash1293 Oct 30, 2020

wylieconlon Oct 30, 2020

wylieconlon Oct 29, 2020

flash1293 Oct 30, 2020

wylieconlon Oct 30, 2020

wylieconlon Oct 29, 2020

flash1293 Oct 30, 2020

flash1293 commented Oct 30, 2020

wylieconlon left a comment

wylieconlon Oct 30, 2020

wylieconlon Oct 30, 2020

MichaelMarcialis left a comment

MichaelMarcialis Oct 30, 2020

flash1293 commented Nov 2, 2020

clintandrewhall left a comment

clintandrewhall Nov 2, 2020

flash1293 Nov 3, 2020

clintandrewhall Nov 2, 2020

flash1293 Nov 3, 2020

clintandrewhall Nov 2, 2020

AlonaNadler commented Nov 2, 2020

flash1293 commented Nov 3, 2020

mbondyra commented Nov 3, 2020

flash1293 commented Nov 4, 2020

kibanamachine commented Nov 4, 2020

-export const defaultCustomColors = [
+// This set of defaults originated in Canvas, which, at present, is the primary
+// consumer of this function.  Changing this default requires a change in Canvas
+// logic, which would likely be a breaking change in 7.x.
+export const defaultCustomColors = [

[Lens] Categorical color palettes #75309

[Lens] Categorical color palettes #75309

Conversation

flash1293 commented Aug 18, 2020 • edited Loading

Canvas changes

Charts changes

Lens changes

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Oct 30, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Nov 2, 2020

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlonaNadler commented Nov 2, 2020

flash1293 commented Nov 3, 2020

mbondyra commented Nov 3, 2020

flash1293 commented Nov 4, 2020

kibanamachine commented Nov 4, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunk count

async chunks size

distributable file count

page load bundle size

History

flash1293 commented Aug 18, 2020 •

edited

Loading