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

Updated color blind viz palette #2686

Merged
merged 28 commits into from
Jan 3, 2020
Merged

Updated color blind viz palette #2686

merged 28 commits into from
Jan 3, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Dec 17, 2019

See this PR live: https://cchaos-viz-colors.netlify.com/#/utilities/color-palettes
Also be sure to check them out in the charts docs section: https://cchaos-viz-colors.netlify.com/#/elastic-charts/creating-charts

The new euiPaletteColorBlind palette

Screen Shot 2019-12-18 at 14 15 16 PM

Read the description of how this was derived in this issue and fixes #2238

More quantitative palettes

And docs that show what happens when you pass different values for the number of steps.

Screen Recording 2019-12-17 at 04 43 PM

Breaks and dependencies

This does cause some breaking changes to the color_palette function to be more robust by accepting an array of colors instead of a single start and end color.

Before

color_palette('#FFF', '#000', 5);

Now

color_palette(['#FFF', '#000'], 5);
color_palette(['#FFF', 'yellow', '#000'], 5);

The color_palette function also adds a dependency on chroma.js for its bezier and correctLightness functions.

It also breaks the eui_palettes which used to just be a static array of hex colors. Now they are a function allowing consumers to pass a certain number of steps to interpolate.

Before

palettes.euiPaletteForStatus // Spits out 10 hex colors

Now

palettes.euiPaletteForStatus(5) // Spits out only 5 hex colors

Also, palettes no longer exists as an exported object of palettes, and instead exports the named palettes directly like export const euiPaletteForStatus(). And gets rid of the .colors key so that each function just returns the array of colors.


One could argue that we could/should change the white to color gradients when in dark mode to go from black to color. However, they're not too bad in dark mode as is and I wonder if that would change the meaning of the chart when toggling between themes. Anyhow, possibly/probably out of scope for this PR.

Checklist

  • Check against all themes for compatability in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Copy link
Member

@johnbarrierwilson johnbarrierwilson left a comment

Choose a reason for hiding this comment

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

I'll leave the code-review up to others who are more familiar with EUI than I am. From the design perspective, I think you did a great job balancing the constraints of accessibility with the desire for a strong color palette to work with. 💯

@cchaos
Copy link
Contributor Author

cchaos commented Dec 18, 2019

Actually, lets hold on reviewing this PR at the moment. Based on @formgeist's concerns with changing the hues of some of these colors, I'm in the process of altering the palette again to better match the original hues.

@mdefazio
Copy link
Contributor

This is very impressive. I think the colors work really well.

My comments are more questions than fixes (and outside of the colors themselves):

  1. For the docs, the last color of the multiple metric series was dropping to a new line (at full screen on my MB 15"). Maybe we drop the code variable to a new line or next to the title to try and avoid this happening?
    image

  2. Should we make it a rule that charts should be on white? (or darkest black)? I don't think there are too many places where they aren't already, but I just ran into one in ML where it was on the page background color. While probably an extremely rare case, the #EFEFEF and #F8F8FB might disappear. I think the colors work so i'm more proposing we provide an extra note on the Charts docs.

@ryankeairns
Copy link
Contributor

ryankeairns commented Dec 18, 2019

I've been playing with this a bit this morning, trying to add some saturation back, as my concern is that we're heading down a similar path as the badges.

It seems as though there is some room for adding more saturation while remaining color blind safe...

Screenshot 2019-12-18 08 02 29

... I'll wait to see your next update, thanks!

@cchaos
Copy link
Contributor Author

cchaos commented Dec 18, 2019

@mdefazio Yeah there probably should be a rule mentioned about this somewhere. Though I do hope that most charts aren't actually stretching the colors that much that they really are using the 3 rotationed color blind palette. That's more for how to handle user-generated charts.

@ryankeairns I totally understand your concerns. However, there was private feedback given that our current palette was too saturated (hot) and that we needed to bring it down (quite a bit). #2238 (comment)

I also agree that when you actually start plotting these colors in charts, the saturation becomes a bit blinding. Let's continue this particular discussion over on the issue.

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

This looks amazing! 😍

I prefer the new colors with less saturation because they follow more natural patterns of color. They look like they belong to the same family.

This is a very interesting article about finding the right color palettes for data viz: article

src-docs/src/views/color_palette/color_palette_example.js Outdated Show resolved Hide resolved
@miukimiu
Copy link
Contributor

This is just a suggestion. In order to avoid the border-radius not looking good when we have multiple lines should we just remove it?

color-palettes-border-radius-issue

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

We discussed this as a team along with @VijayDoshi to explain the "breaking" nature of this change. Summary of the discussion:

  1. We're good to merge this as is. It's a big improvement for what exists today.
  2. This has some very limited downsides towards usage in Avatars, badges and the like. @ryankeairns will do some follow up to think up a non-viz, but still varied pallette that has a bit more pop.
  3. This will require a Kibana follow up PR to adjust some hard coded values (KUI, the charting seeds...etc)

Given the later, my suggestion would be to merge after the holidays, but as soon as we get back.

Also... fantastic work. This is really very nice.

@cchaos
Copy link
Contributor Author

cchaos commented Dec 19, 2019

Ok, I've passed through the final round of palette updates and updated the summary of the PR. (Be sure to also check out the chart's palette selector).

This is ready for review if anyone wants to get a head start on it before the holidays though, as @snide said, we won't actually merge until afterward.

@snide
Copy link
Contributor

snide commented Dec 19, 2019

Ha. Yes. I should probably do an actual code review. I didn't do that as part of that approve but I'll do that today/tomorrow.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Question: Why must the palette have a key called colors instead of just passing back the array?

I don't see any reason why we can't just return an array of colors.

src/services/color/color_palette.ts Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor Author

cchaos commented Jan 2, 2020

@ryankeairns I think this was mostly a question for you since you wrote the palette function.

Question: Why must the palette have a key called colors instead of just passing back the array?

Just wondering since needing to remember to add .colors to the end of the function could cause issues. Were you thinking we'd eventually add more things to these palettes than just the array of colors?

src/services/color/color_palette.ts Outdated Show resolved Hide resolved
Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>
@ryankeairns
Copy link
Contributor

@ryankeairns I think this was mostly a question for you since you wrote the palette function.

Question: Why must the palette have a key called colors instead of just passing back the array?

Just wondering since needing to remember to add .colors to the end of the function could cause issues. Were you thinking we'd eventually add more things to these palettes than just the array of colors?

I don't recall any technical reason or intent to add more things to each palette, rather it provided a little readability to clarify exactly what I was requesting/getting returned. Feel free to strip it out.

@cchaos
Copy link
Contributor Author

cchaos commented Jan 2, 2020

Thanks @ryankeairns. I'll go ahead and remove this since there's already a breaking change in the service anyway. I am also going to export the EUI palettes directly/individually so consumers don't have to write palettes.euiPaletteColorBlind() but just euiPaletteColorBlind() since they start with the euiPrefix and the word palette is repetitive. It will mean consumers have to import each one they use instead of all at once, but I think it's better for ease of remembrance.

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM!

@cchaos cchaos merged commit a2fbf75 into elastic:master Jan 3, 2020
@cchaos cchaos deleted the viz-colors branch January 3, 2020 16:19
@thompsongl thompsongl mentioned this pull request Jan 10, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The fourth color in the color blind viz palette does not work in dark mode
7 participants