feat(color palette): implementing different color palettes with d3 #459

majagrubic · 2019-11-25T11:52:25Z

Summary

Implements color palette calcualtions using d3 libraries.
There are 3 different palettes available at the moment:

categorical - includes d3 categorical schemes and EUI palettes
diverging - includes calculating a diverging palette using one of the d3 interpolators
sequential - includes d3 sequential single-hue & multi-hue palettes. Also, includes a possibility to calculate a custom sequential palette given two colors (starting and ending).
cyclical - https://github.com/d3/d3-scale-chromatic#cyclical

This PR does not include an API to be used from the chart itself. This will be added in a subsequent PR.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

codecov-io · 2019-11-25T11:58:28Z

Codecov Report

Merging #459 into master will increase coverage by 0.03%.
The diff coverage is 78.75%.

@@            Coverage Diff            @@
##           master    #459      +/-   ##
=========================================
+ Coverage   83.77%   83.8%   +0.03%     
=========================================
  Files         158     171      +13     
  Lines        4703    5027     +324     
  Branches      952    1017      +65     
=========================================
+ Hits         3940    4213     +273     
- Misses        748     798      +50     
- Partials       15      16       +1

Impacted Files	Coverage Δ
src/specs/settings.tsx	`100% <ø> (ø)`	⬆️
...ypes/xy_chart/state/selectors/get_color_palette.ts	`36.84% <36.84%> (ø)`
src/state/selectors/get_chart_theme.ts	`77.77% <66.66%> (-5.56%)`	⬇️
src/utils/colors/color_palette.ts	`96.15% <96.15%> (ø)`
.../chart_types/xy_chart/crosshair/crosshair_utils.ts	`90.97% <0%> (-2.21%)`	⬇️
src/utils/commons.ts	`100% <0%> (ø)`	⬆️
..._types/xy_chart/state/selectors/get_cursor_line.ts	`100% <0%> (ø)`	⬆️
src/mocks/series/series.ts	`81.96% <0%> (ø)`
src/mocks/specs/specs.ts	`66.66% <0%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cb0beb...a38f76c. Read the comment docs.

markov00

I've left few comments to avoid adding ts-ignore and fix unexpected results

markov00 · 2019-11-25T15:56:46Z

src/utils/colors/color_palette.ts

+import { interpolateHcl, quantize, interpolateLab } from 'd3-interpolate';
+import { scaleSequential, scaleLinear } from 'd3-scale';
+
+export type ColorPaletteName = 'blues' | 'greens' | 'greys' | 'oranges' | 'purples' | 'reds';


can we add also the support for the multi-hue palettes? https://github.com/d3/d3-scale-chromatic#sequential-multi-hue

I don't know how are we going to explain PuBu and PuBuGn color palette names 😂

src/utils/colors/color_palette.ts

markov00 · 2019-11-25T16:01:07Z

src/utils/colors/color_palette.ts

@@ -0,0 +1,90 @@
+import * as d3ScaleChromatic from 'd3-scale-chromatic';
+import * as d3Color from 'd3-color';


you can also import only {rbg, lab}

I think prefixing the function with d3- equivalents improves readability of the code, but if there are performance concerns around importing the whole namespace, I'm happy to do it.

Is not about namespace, but avoid importing all the d3-color library to allow an optimized treeshaking of the library.
You can still use prefix d3 function if you prefer with import { rgb as d3Rgb } from 'd3-color'

Another reason for individual, aliased D3 imports in repos that are not all-in D3 applications - it's a convenient way to find all entry points to D3 use, just by grepping d3_ such as d3_rgb. The underscore might have a slight edge:

super precise grepping, as d3_ is unlikely to occur elsewhere (while d3 is noisy - it also occurs in colors, UUIDs, tokens, Base64 encoded files eg. encoded SVG, yarn integrity and other hash numbers etc.)

there's no change to the capitalization of the D3 library functions

the _ is slightly weird visually (though easier to spot), but so is the presence of a number inside a camel3Cased identifier

Having said this, either prefix is OK for me (or even their lack thereof), the main point is a slight preference for named, aliased, easy to find imports from D3 in code where it is more of a utility. Sometimes I put all D3 functions into a single file eg. d3utils.ts for some given functionality so it's easy to version bump or replace any of them centrally, if needed.

src/utils/colors/color_palette.ts

markov00 · 2019-11-25T16:32:01Z

src/utils/colors/color_palette.ts

+export function getCategoricalPalette(name: CategoricalSchemeName): ReadonlyArray<string> {
+  const schemeName: string = transformSchemeName(name);
+  // @ts-ignore
+  return d3ScaleChromatic[schemeName];


what if we import each scheme separately and we use a switch here to avoid adding ts-ignore various places

import { schemeCategory10} from 'd3-scale-chromatic'; switch(schemeName){ case 'schemeCategory10': return schemeCategory10; default: ..... }

That is a lot of hardcoding for something that works fine without it. I am not sure it's worth the benefit.

It's for sure hardcoding, but what if scaleChromatic removes one of their scheme names? we will never catch that issue until someone call the function with that removed scale.
The same happens for the opposite: we add a new Scheme name but it's not exported or available on the scale chromatic library (see for example the typo in the previous comment, one value of the ColorPaletteName was misspelled, with a proper typing we can catch these error much more easily).
Maybe we can find some some way to handle that getting the available keys from d3ScaleChromatic with a type like keyof typeof d3ScaleChromatic but I'm not sure how to apply that to the current implementation

My 1st impression was keyof typeof too, hopefully it can be combined with conditionals - eg. check that the user-specified object key exists on the color object - so type guarding is possible. In this case, if D3 retires a color, it can fall back onto some default palette.

Maybe relying on the simplest test cases to detect if a palette was dropped or renamed by D3 is safer than modeling the set of colors as a black box and falling back onto default colors if some go missing

markov00

I've added few comments on the code after your changes.
Still missing to export the functions in the src/index.ts file

markov00 · 2019-11-29T12:00:04Z

src/utils/colors/color_palette.ts

+export function getCategoricalPalette(name: CategoricalSchemeName): ReadonlyArray<string> {
+  const schemeName: string = transformSchemeName(name);
+  // @ts-ignore
+  return d3ScaleChromatic[schemeName];


It's for sure hardcoding, but what if scaleChromatic removes one of their scheme names? we will never catch that issue until someone call the function with that removed scale.
The same happens for the opposite: we add a new Scheme name but it's not exported or available on the scale chromatic library (see for example the typo in the previous comment, one value of the ColorPaletteName was misspelled, with a proper typing we can catch these error much more easily).
Maybe we can find some some way to handle that getting the available keys from d3ScaleChromatic with a type like keyof typeof d3ScaleChromatic but I'm not sure how to apply that to the current implementation

src/utils/colors/color_palette.ts

markov00 · 2019-11-29T12:13:06Z

src/utils/colors/color_palette.ts

@@ -0,0 +1,90 @@
+import * as d3ScaleChromatic from 'd3-scale-chromatic';
+import * as d3Color from 'd3-color';


Is not about namespace, but avoid importing all the d3-color library to allow an optimized treeshaking of the library.
You can still use prefix d3 function if you prefer with import { rgb as d3Rgb } from 'd3-color'

src/utils/colors/color_palette.ts

monfera · 2019-11-29T17:30:09Z

src/utils/colors/color_palette.ts

+  return d3ScaleChromatic[schemeName];
+}
+
+export function getCustomCategoricalPalette(colors: string[], steps: number) {


Not sure if within this PR, but it'd be neat to brainstorm about names, eg. it's a great name overall (no abbreviations to forget etc.) so the only question is, is the get adding value. Usually, functions are equivalent (replaceable by) their return value, or to put it differently, I think it should be the norm rather than the exception that a function is referentially transparent, just doing its thing of returning some value determined by its input. Calling it customCategoricalPalette would suggest that we're doing value based programming rather than place oriented programming

since we have this here and there in the whole library, I think it's ok for now keep the name like this

monfera · 2019-11-29T17:39:22Z

src/utils/colors/color_palette.ts

+  return paletteColors;
+}
+
+export function getSequentialPalette(name: ColorPaletteName, steps: number) {


It looks like all current ways for getting the colors require the specification of the steps, materializing into a array. We should be able to use continuous color scales too, is it out of scope for this PR? May be useful to add eg. a function that'd just take a D3 continuous color ramp, a fromValue and a toValue and it'd return a function that can be called with arbitrary numbers between the two values.

Maybe we can have that as a next-phase PR

majagrubic · 2019-12-02T19:54:12Z

I've incorporated your comments and added a better check if the color palette exists in d3 namespace. Also, got rid of ts-warnings. Rebasing against master and marking this as ready for review.

Importing only relevant d3 modules

Adding implementation for the multi-hue palette

Adding diverging interpolators; adding fail-safe mechanism to get d3-interpolators; getting rid of ts-ignore warnings;

markov00

Thanks for the efforts on this.
I've left few comments, after handling those I think we can merge this.
I think we can also remove the prototyping term in the PR title

markov00 · 2019-12-04T09:43:02Z

package.json

+    "@types/d3-color": "^1.2.2",
+    "@types/d3-interpolate": "^1.3.1",
+    "@types/d3-scale-chromatic": "^1.3.1",


I don't think we need to have the type deps here since we are not exposing any function, return type or type that comes directly from d3. I think it's safe to move them under devDependencies

markov00 · 2019-12-04T09:44:25Z

src/utils/colors/color_palette.ts

+  return d3ScaleChromatic[schemeName];
+}
+
+export function getCustomCategoricalPalette(colors: string[], steps: number) {


since we have this here and there in the whole library, I think it's ok for now keep the name like this

markov00 · 2019-12-04T09:47:01Z

src/utils/colors/color_palette.ts

+  const paletteColors = [];
+  const scale = quantize(interpolateHcl(colors[0], colors[1]), steps);
+  for (let i = 0; i < steps; i++) {
+    paletteColors.push(d3Rgb(scale[i]).hex());
+  }
+  return paletteColors;
+}
+


A categorical palette is composed by a set of colors where each one differs from the other like the EUI palette or the any other palette in https://github.com/d3/d3-scale-chromatic#categorical.
Interpolating the HCL for the first two colors create a sequential scale rather then a categorical one.
I think we should remove this function and let the consumer just input a custom array of different color if they want a custom categorical palette

sequential will have very similar colors in the range. the quantize in the categorical will make sure that the colors are uniformly distributed, which will produce very distinct color. plus, the first and last color in the categorical palette will be the same as user defined, while in the sequential it will be an interpolated value.
Example, for the input colors: ['#007AFF', '#FFF500'] and 10 steps:

generated categorical palette:

generated sequential palette:

It's true that it's sequential in nature, but I think it's still useful?

I see that more as a sequential, multi hue palette than a categorical/qualitative color scale.
It's indeed a nice palette, but doesn't provide the same function as a categorical palette. In the categorical, the color should be easily distinguishable between each other, in your example i can barely distinguish between the 3 yellow, 3 purples, 2 blues, one orange.
I personally don't see that in line with the description of a categorical scale:
http://colorbrewer2.org/learnmore/schemes_full.html#qualitative
https://seaborn.pydata.org/tutorial/color_palettes.html

markov00 · 2019-12-04T09:48:09Z

src/utils/colors/color_palette.ts

+  return paletteColors;
+}
+
+export function getSequentialPalette(name: SequentialColorPaletteName, steps: number) {


can you please add the return type: ReadonlyArray<string> to be aligned with the getCategoricalPalette

markov00 · 2019-12-04T09:48:16Z

src/utils/colors/color_palette.ts

+  return paletteColors;
+}
+
+export function getCustomSequentialPalette(colors: string[], steps: number) {


can you please add the return type: ReadonlyArray<string> to be aligned with the getCategoricalPalette

markov00 · 2019-12-04T09:48:35Z

src/utils/colors/color_palette.ts

+  return paletteColors;
+}
+
+export function getCyclicalPalette(name: CyclicalPaletteName, steps: number) {


return type: ReadonlyArray<string>

markov00 · 2019-12-04T09:49:22Z

src/utils/colors/color_palette.ts

+  return paletteColors;
+}
+
+export function getDivergingPalette(steps: number, interpolatorName?: DivergingInterpolatorName) {


return type: ReadonlyArray<string> and interpolator default to spectral directly in the arguments

Removing custom categorical palette; Adding Readonlyarray as a return type

Re-adding removed d3 librarires

cchaos · 2019-12-04T15:21:06Z

src/utils/colors/color_palette.ts

+  | 'euiPaletteForLightBackground'
+  | 'euiPaletteForDarkBackground'


Can we remove these from being an option? They're specific to Elastic branding and statefulness and shouldn't really be used for charts.

cchaos · 2019-12-04T15:25:13Z

src/utils/colors/color_palette.ts

+import { interpolateLab } from 'd3-interpolate';
+import { scaleLinear, scaleSequential } from 'd3-scale';
+
+export type SequentialColorPaletteName =


There are a lot of options in this list. While choice is great, it can cripple decision and allow for incorrect choices to be made. Can we reduce this list to just a few most useful ones like "warm", "cool"?

Also, are these options going to be documented anywhere with visual examples/representations? I have no idea what cubehelixDefault would result as and consumers would need to apply the palette, compile their app, then view the chart (and hope they have a full number of series) in order to see the palette.

Yes, we'll have a storybook example with palettes being applied to charts and users will get to try it out. This is just the first step of the implementation.
Caroline, I agree with your suggestion that we keep some of the palettes - but the problem here is that I don't really know what "a few most useful" palettes are, as this is quite far from my domain of expertise. Could you please advise on which palettes should be kept and which removed? You can use d3-scale-chromatic for representation of palettes:
https://github.com/d3/d3-scale-chromatic

All color palettes, except EUI are taken from d3 scale chromatic -> https://github.com/d3/d3-scale-chromatic
We will document all of them in storybook (we are going to improve the docs soon #457)
If you like to select just some of them for us, please feel free

cchaos · 2019-12-04T15:28:26Z

src/utils/colors/color_palette.ts

+  | 'set1'
+  | 'set2'
+  | 'set3'


What are these generically named palettes?

These are the categorical palettes that d3 has to offer: https://github.com/d3/d3-scale-chromatic#schemeSet1
For simplicity, I decided to leave out the scheme prefix.

the naming comes from https://github.com/d3/d3-scale-chromatic schemas

cchaos · 2019-12-04T16:11:57Z

Could you please advise on which palettes should be kept and which removed?

My advice is to use some sort of object/map that names these color palettes based on their source but also have a few common and shortly named palettes for easy use.

For example:

Commonly named palettes

Charts would provide a simple list of commonly named palettes like "temperature", "warm", "cool", "grayscale", "category". These would map to hard-coded values provided by Charts (which could also be taken from the D3/EUI options).

The consumer would then write:

const theme = {
  colors: {
    vizColors: 'temperature',
  },
};

What these common palettes are still need to be determined. I'll get some thoughts started on this.

D3 palettes

Providing an easy way for consumers to use these D3 palettes is nice, especially for applications that also allow users to select their own. However, for Elastic engineers we would like to have them stick to the common palettes or EUI palettes so we're consistent throughout.

But for consumers to understand that some palettes are coming from D3, they should have to write it like:

const theme = {
  colors: {
    vizColors: d3Palletes.schemeSet1,
  },
};

And the names shouldn't change from the way they're named from D3. This gives consumers the reference point from where these palettes are coming from for them to go view/find them.

EUI palettes

This will also give us the opportunity to build EUI custom palettes within EUI and provide them to consumers of both EUI and Charts.

const theme = {
  colors: {
    vizColors: euiPaletteForStatus, // Imported from EUI not Charts
  },
};

Custom palettes

And of course still being able to create custom palettes by providing 2+ colors to interpolate.

const theme = {
  colors: {
    vizColors: getCustomSequentialPalette(['#FFFFE0', '#017F75'], 5),
  },
};

cchaos · 2019-12-06T17:57:07Z

I have done some looking at palettes and reached out to the other designers (doc) to get consensus on a few "common" palettes.

I've then come up with a mapping of these common palettes to the D3 options (or custom) to use as defaults straight from Elastic charts. Links to palettes found in this section of the doc.

Palettes = {
  'categorical': d3.schemeCategory10 || euiPaletteColorBlind, // The D3 version is not color blind safe
  'grayscale': d3.schemeGreys[k]
  'status': d3.schemeRdYlGn[k]
  'temperature': d3.schemeRdYlBu[k]
  'warm': d3.schemeOrRd[k]
  'cool': d3.schemePuBu[k]
  'complimentary': custom(#a97143, #f1a261, #dddddd, #5795d5, #3d6995)
}

In EUI, we will make our own version of the same object using the EUI (Elastic) color palette and ensuring better accessibility.

Consumers of both EUI and Elastic charts can then pass in the EUI version of the palette. Or consumers can grab any of the other D3 palettes or create their own.

majagrubic · 2019-12-09T18:02:34Z

@cchaos one question:
what should custom (#a97143, #f1a261, #dddddd, #5795d5, #3d6995)
return?
should we just return that array of colors? if we're gonna do custom sequential palette, we need a start and end color, not an array of 5 colors

cchaos · 2019-12-09T20:14:20Z

So the custom sequential palette function doesn't allow you to specify more than 2 colors? Hmm, that seems pretty limiting given the fact that you might have at least 3 colors to interpolate from (start, middle, end) so that you can specify the middle to be gray (for example).

Adding a layer above current color_palette implementation

majagrubic · 2019-12-10T10:03:41Z

src/chart_types/xy_chart/state/selectors/get_color_palette.ts

+
+function getColorPalette(colorPalette: ColorPalette): ReadonlyArray<string> {
+  const { name, steps } = colorPalette;
+  switch (name) {


Depending on the name, we get the actual color palette.

majagrubic · 2019-12-10T10:03:58Z

src/state/selectors/get_chart_theme.ts

+  } else {
+    finalTheme = theme ? mergeWithDefaultTheme(theme, base) : base;
+  }
+  if (colorPaletteSpec) {


If color palette exists, it's applied here.

majagrubic · 2019-12-10T10:04:57Z

src/utils/colors/color_palette.ts

+export type ColorPaletteName =
+  | 'categorical'
+  | 'colorBlind'
+  | 'grayscale'
+  | 'status'
+  | 'temperature'
+  | 'warm'
+  | 'cool'
+  | 'complimentary';
+
+export type ColorPalette = {
+  name: ColorPaletteName;
+  steps?: number;
+};


These are the user facing palettes.

nickofthyme · 2020-11-16T15:10:12Z

I think this issue will be handled now in kibana via the new palette service in the charts plugin. If not we can revisit these changes in the future, but this PR is too out of date to continue.

cc: @markov00

markov00 reviewed Nov 25, 2019

View reviewed changes

majagrubic force-pushed the protoype-d3-color-palette branch from 5fb0fc6 to ca22637 Compare November 26, 2019 10:08

markov00 reviewed Nov 29, 2019

View reviewed changes

monfera reviewed Nov 29, 2019

View reviewed changes

majagrubic force-pushed the protoype-d3-color-palette branch from 05c5c1a to 47bbd4b Compare December 2, 2019 20:02

Maja Grubic added 3 commits December 2, 2019 20:17

feat(color palette): prototyping different color palettes with d3

a073f57

refactor(dependencies): removing d3 dependency

7a87d10

Importing only relevant d3 modules

refactor(color_palette): applying PR comments

e5bd56c

Adding implementation for the multi-hue palette

majagrubic force-pushed the protoype-d3-color-palette branch from 47bbd4b to bec9d1e Compare December 2, 2019 20:28

refactor(color_palette): applying PR comments

fdea1e3

Adding diverging interpolators; adding fail-safe mechanism to get d3-interpolators; getting rid of ts-ignore warnings;

majagrubic force-pushed the protoype-d3-color-palette branch from bec9d1e to fdea1e3 Compare December 2, 2019 21:05

majagrubic marked this pull request as ready for review December 2, 2019 21:34

majagrubic requested review from markov00 and monfera December 3, 2019 19:16

markov00 reviewed Dec 4, 2019

View reviewed changes

refactor(color_palette): applying PR comments

6222a33

Removing custom categorical palette; Adding Readonlyarray as a return type

majagrubic changed the title ~~feat(color palette): prototyping different color palettes with d3~~ feat(color palette): implementing different color palettes with d3 Dec 4, 2019

Maja Grubic added 2 commits December 4, 2019 14:27

Merge branch 'master' into protoype-d3-color-palette

bd6288d

refactor: adding missing dependencies

c1d7d2b

Re-adding removed d3 librarires

cchaos suggested changes Dec 4, 2019

View reviewed changes

refactor(color_palette): removing EUI color palettes

0efc13a

improvement(color_palette): exposing color_palette in the API

a38f76c

Adding a layer above current color_palette implementation

majagrubic commented Dec 10, 2019

View reviewed changes

markov00 mentioned this pull request Dec 10, 2019

Handling colors #492

Open

markov00 force-pushed the master branch from c901523 to 6bf9a69 Compare March 2, 2020 18:53

markov00 added the stale stale PR or issue label Mar 5, 2020

nickofthyme closed this Nov 16, 2020

		@@ -0,0 +1,90 @@
		import * as d3ScaleChromatic from 'd3-scale-chromatic';
		import * as d3Color from 'd3-color';

		\| 'euiPaletteForLightBackground'
		\| 'euiPaletteForDarkBackground'

feat(color palette): implementing different color palettes with d3 #459

feat(color palette): implementing different color palettes with d3 #459

Conversation

majagrubic commented Nov 25, 2019 • edited Loading

Summary

Checklist

codecov-io commented Nov 25, 2019 • edited Loading

Codecov Report

markov00 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 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

majagrubic commented Dec 2, 2019 • edited Loading

markov00 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

majagrubic Dec 4, 2019 • edited Loading

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

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

majagrubic Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Dec 4, 2019

Commonly named palettes

D3 palettes

EUI palettes

Custom palettes

cchaos commented Dec 6, 2019

majagrubic commented Dec 9, 2019

cchaos commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme commented Nov 16, 2020

majagrubic commented Nov 25, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited

Loading

majagrubic commented Dec 2, 2019 •

edited

Loading

majagrubic Dec 4, 2019 •

edited

Loading

majagrubic Dec 4, 2019 •

edited

Loading