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

Adds colorPalette service #1209

Merged
merged 5 commits into from
Sep 25, 2018

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Sep 24, 2018

Summary

Adds a service that accepts a palette name and returns an array of hexidecimal color codes for use in other EUI components such as charts.

You can either...
a) pull a palette by name from the eui_palettes file,
b) provide your own palettes file, or
c) use the "custom" palette name along with two hex color codes and a number of stops for a gradiated array of hex color codes.

localhost_8030_

Checklist

  • This was checked in mobile - There is no UI other than docs which was checked on mobile.
  • This was checked in IE11
  • This was checked in dark mode - There is no UI however a lighter version of the eui_colors palette has been added.
  • Any props added have proper autodocs - No props since it's a service, but documented code
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios - There is no UI as this is a service/function.

@ryankeairns ryankeairns self-assigned this Sep 24, 2018
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Before a full code review, I just had a couple questions.

<p>
Generate a custom palette of any length from two hexidecimal color
codes such as
<EuiCode>colorPalette&#40;&#39;custom&#39;, &#39;FF0000&#39;, &#39;#00FFFF&#39;, 25&#41;</EuiCode>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this service need the hash mark for hex colors? I see you have it both ways here, just wondering if that's a typo or if the service accepts both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, instead of having to pass 'custom' as the first value, can we make the service smart enough to recognize if the first param is a named palette or a color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can pass it with or without the # so I was just demonstrating that in the examples... then copy/paste happened and it's now in all three examples :D . I'll look into the latter suggestion, I set that aside initially and didn't return to it.

@chandlerprall chandlerprall self-requested a review September 24, 2018 15:26
@ryankeairns ryankeairns requested review from snide and cchaos September 24, 2018 15:29
@ryankeairns ryankeairns changed the title [WIP] Adds colorPalette service Adds colorPalette service Sep 24, 2018
@ryankeairns
Copy link
Contributor Author

ryankeairns commented Sep 24, 2018

There is a known console warning (repeating 6x) that originates from the chart example. Several EUI chart components validate the color code against the viz color palette. @cchaos mentioned she removed this PropType, but I've rebased against master and still see the warning.

I might have misunderstood, but either way, feedback on resolving that warning would be welcome as it might result in us removing that validation check (if it hasn't been already) or doing something else like validating based upon a provided palette ('path to file' feels kinda messy), or simply validating against the eui_palettes.js file added in this PR (it also includes the viz/color_blind color palette).

screenshot 2018-09-24 10 43 16

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Also needs unit tests for the service methods.

src/services/color/eui_palettes.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
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.

I gave comments during our weekly. They were mostly docs related.

  1. The docs should provide guidelines and palettes for what we consider good "qualitative" and "quantitative" palettes. The only palettes in the docs should be ones we're comfortable with devs using for visualizations. Any of the other examples should be removed so that people don't use them. We should describe the palettes by saying "This is a good qualitative palette for health measurement", "This is a good one for...", ...etc.
  2. The EUI palette needs to show "EUI Light" and "EUI Dark", which are just mirrors of our sass ones. Currently "EUI light" in this iterative is a slant of what we already provide and is safe to remove.

@cchaos
Copy link
Contributor

cchaos commented Sep 24, 2018

[ I ] mentioned she removed this PropType, but I've rebased against master and still see the warning.

That's my bad, I didn't realize that the color props were also expecting only one of the Vis colors. What I was referring to was removing this warning: https://github.com/elastic/eui/pull/1198/files#diff-1107a4e514bdba21c291b3d986be0217

I think it's safe to change those prop types to string, unless there's a better way to restrict to a color of hex type.

@ryankeairns
Copy link
Contributor Author

Updates:

  • Removed eui_palettes.js from colorPalettes function. The preset qualitative color palettes are now simply loaded directly from the eui_palettes.js file.
  • Added and tweaked preset palettes
  • Updated sample quantitative (generated) palettes
  • Re-titled demo sections and added further explanation
  • Added additional chart examples (one with preset palette; one with generated)
  • Changed the PropType on all other chart components to simply validate as strings as opposed to checking against the viz color set since you can now use these preset or customer palettes

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

few more small changes

src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
src/services/color/color_palette.js Outdated Show resolved Hide resolved
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.

Did a more thorough code review. Can't wait to use this stuff. My comments are pretty trivial and mostly deal with presentation.

Basically I'd think of this page less as "here's a way to build palettes" and more "this is what palettes design has said are good to use".

src-docs/src/views/color_palette/color_palette_custom.js Outdated Show resolved Hide resolved
src-docs/src/views/color_palette/color_palette_custom.js Outdated Show resolved Hide resolved
src-docs/src/views/color_palette/color_palette_custom.js Outdated Show resolved Hide resolved
src/services/color/eui_palettes.js Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

changes LGTM, pulled and tested docs

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.

All good! Yes. This crosses one off of our roadmap 🗡

@ryankeairns ryankeairns merged commit 584fdd2 into elastic:master Sep 25, 2018
@snide snide mentioned this pull request Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants