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 inline and TS conversion #2340

Merged
merged 10 commits into from
Sep 16, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Sep 13, 2019

Summary

In preparation for the color stops component (#2344; see #2028 for discussion), EuiColorPicker needs to be able to render a standalone picker (no bound input or popover). This adds an inline boolean prop to support the required behavior.

Also, more of the EuiColorPicker dependencies have been converted to TS (all but one!), so I've made it TS as well.

Checklist

- [ ] Checked in dark mode
- [ ] 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
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.

Couple small nits, but otherwise LGTM!

src/components/color_picker/color_picker_swatch.tsx Outdated Show resolved Hide resolved
src/components/color_picker/saturation.tsx Outdated Show resolved Hide resolved
/**
* Renders inline, without an input element or popover
*/
inline?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern is having yet another boolean here. Is there anyway we can combine with mode or button since having a button with an inline version doesn't make any sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combining with button makes some sense (like, passing false makes it inline). mode doesn't make sense, though, as its options are still valid with inline rendering (like, an inline swatch-less color picker).

I mostly went with the new boolean for consistency, because that's what EuiDatePicker has

Copy link
Contributor

Choose a reason for hiding this comment

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

@snide And I have talked about it a lot lately and we're tend box ourselves in when we create new props that are straight booleans. Sometimes they're exclusive and sometimes they're not. It's almost better, if you don't see a logical place to add to a current prop, to create an enum style prop instead of boolean. Like for this one it could be display (our tried and true):

display: 'inline' | 'popover'

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. Tried it out, works as expected including keyboard navigation. It would be helpful to display the current hex value in this example, but that could be tackled later.

@thompsongl thompsongl merged commit 8ba1b88 into elastic:master Sep 16, 2019
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