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

[EuiProgress] Add support for more colors #4130

Merged
merged 20 commits into from
Oct 26, 2020

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Oct 13, 2020

Summary

  • Added more colors to EuiProgress (tint1 to tint9), warning and success.
  • Updated the docs example to show available colors.

Note: Colors success and secondary use the same color, I've left secondary as an option thinking of consumers that might already be using secondary in their current implementations.

Fixes #4117 #3618

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs 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

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

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.

This is a nice expansion of the color prop to allow consumers to easily color with our visualization palette. 😍

The one thing that's not quite addressed in this PR that #3618 is asking for, is to be able to completely customize the color by passing a valid hex or rgb or other valid color type. That's where this can get complicated.

I wonder if it can be done similarly to EuiIcon, where a custom color gets applied in the inline style tag as the text color like style={{ color: customColor }} and then in the CSS we use currentColor to color the fill.

The only thing is that we couldn't then apply the makeHighContrastColor() mixin for the text color, but we could either caveat the example description with this or we do have a way to increase the contrast of a color in JS for just the text portion.

src-docs/src/views/progress/progress_size_color.js Outdated Show resolved Hide resolved
src/components/progress/progress.tsx Outdated Show resolved Hide resolved
src/components/progress/_variables.scss Show resolved Hide resolved
@andreadelrio
Copy link
Contributor Author

This is a nice expansion of the color prop to allow consumers to easily color with our visualization palette. 😍

The one thing that's not quite addressed in this PR that #3618 is asking for, is to be able to completely customize the color by passing a valid hex or rgb or other valid color type. That's where this can get complicated.

I wonder if it can be done similarly to EuiIcon, where a custom color gets applied in the inline style tag as the text color like style={{ color: customColor }} and then in the CSS we use currentColor to color the fill.

The only thing is that we couldn't then apply the makeHighContrastColor() mixin for the text color, but we could either caveat the example description with this or we do have a way to increase the contrast of a color in JS for just the text portion.

@cchaos I didn't know about currentColor! that's cool. I've implemented most of your suggestions. The one bit missing is applying makeHighContrastColor() to valueText when they pass a custom color. You mention there's a way to do that in JS? can you point me to an example?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

@cchaos
Copy link
Contributor

cchaos commented Oct 14, 2020

You mention there's a way to do that in JS? can you point me to an example?

I looked through some of our components where I thought we were using it, but we don't seem to have this exact implementation of changing the actual color. So I think we could just put a callout in the docs saying that we don't do any adjustment for contrast when applying the custom color to the text.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

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.

Updates are great. Here's a code review.

Looks like we're missing tests for the color prop. You'll also want to be sure to add one specifically for the custom color as well.

CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/views/progress/progress_colors.js Outdated Show resolved Hide resolved
src-docs/src/views/progress/progress_colors.js Outdated Show resolved Hide resolved
src-docs/src/views/progress/progress_colors.js Outdated Show resolved Hide resolved
src-docs/src/views/progress/progress_colors.js Outdated Show resolved Hide resolved
src/components/progress/_variables.scss Show resolved Hide resolved
src/components/progress/_progress.scss Outdated Show resolved Hide resolved
src/components/progress/progress.tsx Outdated Show resolved Hide resolved
src/components/progress/progress.tsx Outdated Show resolved Hide resolved
src/components/progress/progress.tsx Outdated Show resolved Hide resolved
Co-authored-by: Caroline Horn <[email protected]>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

@andreadelrio
Copy link
Contributor Author

@cchaos thanks for the review. I addressed the feedback and added tests (we were also missing one for size).

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.

Nice! Looks great! I tested in Chrome, FF, and Safari and they all render the proper color. 👍

@chandlerprall Want to take a quick peak at the JS side?

src-docs/src/views/progress/progress_example.js Outdated Show resolved Hide resolved
@@ -95,4 +95,24 @@ describe('EuiProgress', () => {

expect(component).toMatchSnapshot();
});

describe('color', () => {
[...COLORS, '#885522'].forEach((color) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🌟

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

@cchaos
Copy link
Contributor

cchaos commented Oct 19, 2020

I think CI is failing on Prettier of the progress_example.js file. Might want to do a quick re-save of that file to get it formatted.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

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!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4130/

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.

[EuiProgress] Include more names in EuiProgress's colorToClassNameMap
4 participants