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 #1914

Merged
merged 74 commits into from
May 31, 2019
Merged

EuiColorPicker #1914

merged 74 commits into from
May 31, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented May 3, 2019

Summary

🎨 Replaces EUI's third-party color picker component with a custom, swatch-friendly, accessible, HSV picker.

Maintains API compatibility with previous component:

  • Input: hex; accepts nil
  • Output: hex
  • Layout: Single block-level element w/ popover
  • Support: No alpha channel
  • Users lose ability to toggle HSL & RGB input

Tests remain to be written. I can work on that while functionality is reviewed.
Please do a manual screen reader test. I had trouble with Voiceover + Chrome, but want second opinions. Other browsers work as I would expect given WAI-ARIA guidelines.

Background

Builds on #856 (swatch-based picker).
Issues immediately addressed (#1847):

Future

  • Accept more input formats: rgb(a), hsl, hsv, CSS color names
  • Provide conversion service for specific output values
  • Add/remove swatches
  • Allow customization: turn on/off swatches, hue slider, etc. DONE

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • 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
  • This required updates to Framer X components

@snide
Copy link
Contributor

snide commented May 7, 2019

@thompsongl Here's a design PR thompsongl#3

Changes

image

  • Color picker now appears flush to form when used with an input
  • Swatch now has an inset / border so that a same color background (ex: white on white) appears
  • Tightened up the spacing
  • Enunciated the focus state of the hue slider thumb since it has trouble with the focus ring animation.
  • Conformed things to our sizing / spacing
  • Added another button example

@snide
Copy link
Contributor

snide commented May 30, 2019

I do think we'd need a visual cue to compliment the VO cue for keyboard navigators not using VO

This makes sense. If you're turning off the auto open, then you'll need something that opens it up. I'm torn between thinking that the caret should be a button or whether it should be purely visual and you hit something like "downarrow" for trigger the popover.

Either way I think these are fairly minor issues. Let's remember this thing closes a bunch of tickets and is overall a major improvement. It might make sense to make an issue, live with it for a week and get feedback, then circle back. Having to hit tab -> ESC right now is not the end of the world.

Copy link
Contributor

@PhilippBaranovskiy PhilippBaranovskiy left a comment

Choose a reason for hiding this comment

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

I'm watching this PR already for a while, that's a great work, Greg!
I tested the last version (right before leaving this comment),
I've discovered one issue:

This is input. Tab to this.
Color picker is auto popup (expanded).
Hit the TAB key, => that moves the focus to literally color picker small rounded point.

Ok, I've chosen the right color, I'm ready to go next.
I'm done with that popover, [1] hit ESC :) => that moves the focus back to the input.

Ok, I have my color in the input, the popover is open, but that's ok, I'm not going to use it (I don't care because it's auto-expanded, so I'm thinking if I don't go inside, it will auto self-close).

So, yes, I have my color in the input, I'm ready to go next,
hit TAB key, => I'm focusing on rounded small color picker, again!
No, hit ESC => focus on the input, hit TAB => I'm on the rounded small point, hit ESC.... and so forth.


[1] yes, it may seem obvious that to confirm your choice you need to hit ENTER, however, when you know that the input value is already changed, you can decide to close popover.
And it sounds natural that ESC key is the right key to close something.

@thompsongl
Copy link
Contributor Author

@rockfield Thanks for the testing!

This seems to be specifically an issue with VO focus and keyboard focus being on different elements. I think making ESC and ENTER behave the same will fix it, so I'll try that.

@thompsongl
Copy link
Contributor Author

Hmm there must be something else going on in @rockfield's description, because ESC doesn't have that behavior for me. Almost certainly a macOS vs Windows thing?

@snide
Copy link
Contributor

snide commented May 31, 2019

@thompsongl It's definitely windows / IE. You need to double escape to get out. http://snid.es/a3d96445ca76

Works fine on mac / chrome.

@thompsongl
Copy link
Contributor Author

Ah yeah ok. This is an IE bug that I fixed and must have reintroduced.

@PhilippBaranovskiy
Copy link
Contributor

hm... I didn't know anything about IE, => I tested that on windows indeed, however on Firefox.
Now I confirm that it works as expected with last two commits:

  • fix i18n default
  • ESC as ENTER (the last one at the moment I'm writing this)

@thompsongl
Copy link
Contributor Author

Alright.

It might make sense to make an issue, live with it for a week and get feedback, then circle back.

If we go this ^ route, then I think this is ready to merge.
If anyone feels strongly that an explicit open action should be part of the initial release, then let's discuss that design.

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.

Verified IE from latest commit and functionally I'm OK with where this is at currently. HUGE PR.

@ryankeairns
Copy link
Contributor

Looking great, fantastic work!

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.

I'm good with fixing the tab+focus thing as a separate issue

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