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

EuiColorPicker opacity support #2850

Merged
merged 37 commits into from
Mar 12, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Feb 12, 2020

Summary

Resolved the primary goal of #2629.

Screen Shot 2020-02-12 at 1 24 05 PM

  • EuiColorPicker color prop and text input will now accept hex and RGB(a) values, automatically parsing opacity and format.
  • EuiColorPicker onChange now has a second parameter, which is an object containing the rgba and hex representations of the input (('#hex', [r,g,b,a])), as well as an isValid flag.
  • Enabling:
    • Opacity of off by default
    • Enabling showOpacity will render the range input selector

Coming soon(ish)

I think there will be 1 additional PR after this:
Add the option to render a hex input in the popover, as seen in #2629 (comment). May involve a refactor of EuiColorStops.

Checklist

  • Check against all themes for compatibility 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

@thompsongl thompsongl requested a review from cchaos February 12, 2020 19:26
@chandlerprall
Copy link
Contributor

Maybe render the color over a repeating checkboard pattern to show the transparency?

@cchaos
Copy link
Contributor

cchaos commented Feb 12, 2020

Maybe render the color over a repeating checkboard pattern to show the transparency?

I think that only works for large images. The swatches are so small, that this will just cause noise and confusion as to what's going on. Just a simply applying the opacity to the swatch is ideal.

@cchaos
Copy link
Contributor

cchaos commented Feb 12, 2020

Currently, the swatch next to the hex input is the only place that actually shows the color with opacity applied (see screenshot above).
Is this confusing?

No I think we're fine here but I do think the hex value in the input should be the full 8-digit representation. Right now there's no indictor of the opacity value in the input.

Should all visual indications of the selected color be shown with opacity applied?

No, just the output (indicator in the text input)

If opacity is less than 100% and I click a prepopulated swatch from the list, should opacity be reset to 100%?

Yes but just from the pre-populated swatches which could also contain their own opacity values.

@thompsongl
Copy link
Contributor Author

thompsongl commented Feb 13, 2020

Thought about this some more after a question from @cchaos. I think this should instead:

  • accept 6- and 8-digit hex values, as well as rgb and rgba...
  • from which the alpha value can be parsed...
  • which eliminates the need for a separate alpha prop

We could go with 6- and 8-digit hex support only for now, but I doubt consumers have and can use 8-digiit hex values without further conversion (IE11 support, for instance). rgba input support seems necessary for automatic alpha value parsing.

This approach would also prevent the breaking change to EuiColorStops mentioned in the summary.

@thompsongl
Copy link
Contributor Author

^ Still have quite a bit of clean-up to do (e.g., validation in the docs), but you can give it a test drive now.

Still thinking on:

  • Returning either hex or rgba depending on what input was provided originally
  • Returning an isValid boolean as part of onChange to make simple validation easier

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2020

A couple of feedback items from a visual/functional test:

  1. The color stops handle shouldn't show through to the bar underneath. The handle should then have a background of $euiColorEmptyShade.

Screen Shot 2020-02-19 at 11 56 05 AM

  1. Functionally, only clicking the swatches should reset the opacity slider to 100% (or whatever the swatch's alpha is since I'd hope even the swatches can accept alpha channel?)

Screen Shot 2020-02-19 at 11 52 08 AM

  1. The invalid state needs fixing if the input allow alpha channel

Screen Shot 2020-02-19 at 11 51 02 AM

Unrelated

Screen Shot 2020-02-19 at 12 19 57 PM

Screen Shot 2020-02-19 at 11 57 57 AM

Screen Shot 2020-02-19 at 12 01 29 PM

Screen Shot 2020-02-19 at 12 00 00 PM

import { useColorPicker } from './utils';

export const Alpha = () => {
const [color, setColor, errors] = useColorPicker('#D36086');
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like useColorPicker is probably a service we'll want to document and export if it's this helpful in a consuming application?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to open an issue for this. I hadn't thought about it, but it does seem really useful and could be something of a pattern for other relatively simple form-like components.

@thompsongl
Copy link
Contributor Author

@cchaos Still working on item No. 1 (see-through color stops handle), but Nos. 2 and 3 are fixed.

As for the "Unrelated" items:

  • Compressed and invalid states for EuiColorStops: No designs exist for either that I know of. Do we need a ticket for someone to work on these?
  • Beginning and end transition colors: This is intentional. I'm having trouble finding the conversation, but it boils down to is that each color thumb is the start of a new color. Before a thumb, it is either the previous color or empty/gray. After a thumb, that color goes to infinity or until the next thumb.
  • Grayed out indicator in disabled state: Bug. I'll get it fixed.
  • Badge: You get console warnings about it being an invalid color. We can definitely prevent it in the docs, but should EuiBadge itself prevent opacity?

@snide
Copy link
Contributor

snide commented Feb 19, 2020

Beginning and end transition colors: This is intentional. I'm having trouble finding the conversation, but it boils down to is that each color thumb is the start of a new color. Before a thumb, it is either the previous color or empty/gray. After a thumb, that color goes to infinity or until the next thumb.

Yep. This was a valid request from Maps. They have situations where the color doesn't start until a certain point. The best way to illustrate that was to use gray since that felt "disabled"

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2020

Compressed and invalid states for EuiColorStops: No designs exist for either that I know of. Do we need a ticket for someone to work on these?

I think we should just remove the toggles from the docs. You can pass canInvalid: false and canCompressed: false to remove them from DisplayToggles

Badge: You get console warnings about it being an invalid color. We can definitely prevent it in the docs, but should EuiBadge itself prevent opacity?

Yes I think EuiBadge should still warn, but instead of warn AND show a transparent background (which as you can see also invalidates the contrast), it should at least not respect alpha channels. Chroma might have a function that does this?

@thompsongl
Copy link
Contributor Author

Yes I think EuiBadge should still warn, but instead of warn AND show a transparent background (which as you can see also invalidates the contrast), it should at least not respect alpha channels.

Chroma can help with this, for sure. I'm going to open an issue for it, because we'll want to change validation and error messaging at the same time. Somewhat related to #2804

@kibanamachine
Copy link

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

@JoshMock
Copy link
Member

That one, on the other hand, was real! 🎉

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

Looks good from my side! 🎉

@kibanamachine
Copy link

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

@thompsongl thompsongl requested a review from chandlerprall March 4, 2020 19:37
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.

Have a mix of comments, suggestions, and requests. Haven't yet done a full review of the components' internal changes, as I suspect some of this review will impact that area as well.

src/components/color_picker/color_picker.tsx Outdated Show resolved Hide resolved
src/components/color_picker/color_picker.tsx Outdated Show resolved Hide resolved
src/components/color_picker/color_picker.tsx Show resolved Hide resolved
src-docs/src/views/color_picker/formats.js Outdated Show resolved Hide resolved
src-docs/src/views/color_picker/alpha.js Show resolved Hide resolved
@kibanamachine
Copy link

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

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.

Some small suggestions; this looks great though!

src/components/color_picker/color_picker.tsx Outdated Show resolved Hide resolved
src/components/color_picker/color_picker.tsx Show resolved Hide resolved
src/components/color_picker/color_picker.tsx Outdated Show resolved Hide resolved
src/components/color_picker/utils.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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 & tested locally. Thank you @thompsongl for the effort & dedication on this one!

@chandlerprall
Copy link
Contributor

Also: quick changelog entry before merging 🥇

@kibanamachine
Copy link

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

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.

6 participants