Add new react color picker to ui framework with tests #12245

stacey-gammon · 2017-06-08T20:14:35Z

In prep for dashboard color customization - #9243

Tried to match the design team mockups:

But since that didn't include the chooser I just went with something from react-color:

I included a thin black border around the swatch in case the background of the form matches the color picker swatch color (e.g. it's white).

cc @snide

In prep for dashboard color customization.

snide

Excellent library choice. Looks like something easily extendible to whatever we wanna do. Added some super minor comments for the current implementation.

snide · 2017-06-08T20:47:29Z

ui_framework/components/form/_color_picker.scss

+.kuiColorSwatch {
+  width: 19px;
+  height: 19px;
+  border-radius: 3px;


use $globalBorderRadius here.

snide · 2017-06-08T20:48:32Z

ui_framework/components/form/_color_picker.scss

@@ -0,0 +1,29 @@
+.kuiColorSwatch {
+  width: 19px;
+  height: 19px;


Define these as a local var. Should probably be 20px based on other sizing stuff in k5 currently.

A var like this would go inside the _index.scss file in this component's directory, before the imports:

$colorPickerSize: 20px; @import "assisted_input"; @import "check_box"; @import "label"; @import "search_input"; @import "select"; @import "static_input"; @import "text_area"; @import "text_input"; @import "color_picker";

snide · 2017-06-08T20:51:03Z

ui_framework/components/form/_color_picker.scss

+  width: 19px;
+  height: 19px;
+  border-radius: 3px;
+  border: 1px solid black;


Might want a softer semi-transparent color here, so it still shows a border, but isn't as harsh. Something like rgba(0,0,0,.2);

This is a great idea. If we use a box-shadow instead then we can even get the border to blend with the swatch color:

box-shadow: inset 0 0 0 1px rgba(#000, 0.2);

cjcenizal

⚡️ This is so cool. I had some substantial suggestions... please forgive the code invasion! My suggestions began to interleave so I decided the best way to communicate them was with larger-size code snippets.

cjcenizal · 2017-06-09T03:15:51Z

ui_framework/components/form/_color_picker.scss

+  border-radius: 3px;
+  border: 1px solid black;
+  display: inline-block;
+  vertical-align: middle;


Instead of using inline-block we can use flexbox to vertically align the content. It required adding an additional element. When I added this element I ended up adjusting the names of these classes a bit to a) use BEM conventions and b) reflect the relationships among elements a little more clearly (e.g. the connection between the popover and the cover, which I renamed overlay since we're using that term already with the modal).

Here's what the SCSS ended up looking like:

.kuiColorPicker { cursor: pointer; } .kuiColorPicker__preview { display: flex; align-items: center; } .kuiColorPicker__swatch { width: 19px; height: 19px; border-radius: $globalBorderRadius; box-shadow: inset 0 0 0 1px rgba(#000, 0.2); } .kuiColorPicker__label { font-size: $globalFontSize; line-height: $globalLineHeight; margin-left: 10px; display: inline-block; vertical-align: middle; } .kuiColorPickerPopUp { position: absolute; z-index: 10; } .kuiColorPickerPopUpOverlay { position: fixed; top: 0; right: 0; bottom: 0; left: 0; }

cjcenizal · 2017-06-09T03:17:02Z

ui_framework/components/form/color_picker.js

+    this.props.onChange(color.hex);
+  };
+
+  render() {


Here's what the render method looks like using the above classes. I moved the "toggle" click handler onto the root element so the user can click the entire label and swatch instead of just the swatch, since that's a pretty small click area.

render() { const color = this.props.color || '#ffffff'; const classes = classNames('kuiColorPicker', this.props.className); return ( <div className={classes} aria-label={ this.props['aria-label'] } data-test-subj={ this.props['data-test-subj'] } onClick={ this.toggleColorSelector } > <div className="kuiColorPicker__preview"> <div className="kuiColorPicker__swatch" aria-label="Select a color" data-test-subj="colorSwatch" style={{ background: this.props.color }} /> <div className="kuiColorPicker__label" aria-label={`Color selection is ${color}`} > { color } </div> </div> { this.state.showColorSelector ? <div className="kuiColorPickerPopUp" data-test-subj="colorPickerPopup"> <div className="kuiColorPickerPopUpOverlay" onClick={ this.closeColorSelector } /> <ChromePicker color={ color } disableAlpha={ true } onChange={ this.handleColorSelection } /> </div> : null } </div> ); }

cjcenizal · 2017-06-09T03:17:43Z

ui_framework/components/form/_color_picker.scss

+}
+
+.kuiColorLabel {
+  font-size: 16px;


I think we should use $globalFontSize here instead, which will be more consistent with the font-size we're using elsewhere. This is reflected in the SCSS snippet in my comment.

cjcenizal · 2017-06-09T03:20:43Z

ui_framework/components/form/index.js

@@ -0,0 +1 @@
+export { KuiColorPicker } from './color_picker';


Can we move this component into its own color_picker directory? It's such a rich component, I feel like it might make more sense to make it more top-level and leave the form directory for more primitive form components.

cjcenizal · 2017-06-09T03:21:25Z

ui_framework/doc_site/src/views/form/color_picker.js

+  render() {
+    return <KuiColorPicker onChange={ this.handleChange } color={ this.state.color }/>;
+  }
+}


If we make the ColorPicker component live in its own directory, then can we also surface the ColorPicker example as its own page instead of part of the Form page?

cjcenizal · 2017-06-09T03:27:03Z

ui_framework/components/form/color_picker.js

+        {
+          this.state.showColorSelector ?
+            <div className="kuiColorPickerPopUp" data-test-subj="colorPickerPopup">
+              <div className="kuiColorPickerCover" onClick={ this.closeColorSelector } />


Unrelated to my previous comments, I think we should look for a better way to dismiss the popup instead of by intercepting and completely blocking clicks via this popup. Ideally a user should be able to click a button or form input elsewhere on the screen, and simultaneously interact with that element and dismiss this popup. Maybe @zinckiwi could offer some suggestions for us?

stacey-gammon · 2017-06-09T13:33:53Z

After addressing code review comments:

stacey-gammon · 2017-06-13T19:44:16Z

When you have a chance for look, this should be all set to go @cjcenizal!

cjcenizal

One last suggestion based on an idea from @zinckiwi, and a couple small nitpicky things.

cjcenizal · 2017-06-13T22:19:23Z

ui_framework/components/color_picker/_color_picker.scss

+  cursor: pointer;
+}
+
+.kuiColorPicker__preview {


Can we indent kuiColorPicker__preview, kuiColorPicker__swatch, +.kuiColorPicker__label? This visually communicates the parent-child relationship without incurring the specificity you get when you nest them.

cjcenizal · 2017-06-13T22:20:19Z

ui_framework/doc_site/src/services/routes/routes.js

@@ -134,6 +137,10 @@ const components = [{
  name: 'Card',
  component: CardExample,
 }, {
+  name: 'Color Picker',


Minor nit, can we change this to ColorPicker to be consistent with the component name?

cjcenizal · 2017-06-13T22:33:53Z

ui_framework/components/color_picker/color_picker.js

+
+  handleColorSelection = (color) => {
+    this.props.onChange(color.hex);
+  };


Minor nits: can we remove the semicolons from these three methods for consistency with our other method definitions?

Because it's an assignment, not a function, intellij complains without the semi colon.

So I think it's actually more correct to use the semis in this situation.

cjcenizal · 2017-06-13T22:35:40Z

ui_framework/components/color_picker/color_picker.js

+    this.props.onChange(color.hex);
+  };
+
+  render() {


I spoke with @zinckiwi and he suggested a way to support closing the colorpicker without adding an element which will intercept the mouse click. Here's the code:

onClickRootElement = e => { // This prevents clicking on the element from closing it, due to the event handler on the // document object. e.nativeEvent.stopImmediatePropagation(); } componentDidMount() { // When the user clicks somewhere outside of the color picker, we will dismiss it. document.addEventListener('click', this.closeColorSelector); } componentWillUnmount() { document.removeEventListener('click', this.closeColorSelector); } render() { const color = this.props.color || '#ffffff'; const classes = classNames('kuiColorPicker', this.props.className); return ( <div className={ classes } aria-label={ this.props['aria-label'] } data-test-subj={ this.props['data-test-subj'] } onClick={ this.onClickRootElement } > <div className="kuiColorPicker__preview" onClick={ this.toggleColorSelector } > <div className="kuiColorPicker__swatch" aria-label="Select a color" data-test-subj="colorSwatch" style={{ background: this.props.color }} /> <div className="kuiColorPicker__label" aria-label={`Color selection is ${color}`} > { color } </div> </div> { this.state.showColorSelector ? <div className="kuiColorPickerPopUp" data-test-subj="colorPickerPopup"> <ChromePicker color={ color } disableAlpha={ true } onChange={ this.handleColorSelection } /> </div> : null } </div> );

With this change we add a click handler to the document which will close the color picker. When we click on the color picker itself, we prevent the event from reaching this handler; otherwise clicking the color picker would open and then immediately close it. What do you think @stacey-gammon?

@zinckiwi Can you confirm this is what you had in mind?

Works like a charm, thanks @zinckiwi and @cjcenizal!

It turns out that this doesn't work well when there are multiple color pickers available on the same page, and you click from one immediately to the other.

e.nativeEvent.stopImmediatePropagation(); stops the other color pickers from closing.

Have you run into this caveat at all @zinckiwi?

I think I have a solution to get around this

cjcenizal · 2017-06-13T22:35:56Z

ui_framework/components/color_picker/_color_picker.scss

+  z-index: 10;
+}
+
+.kuiColorPickerPopUpOverlay {


We can remove this class based on my other suggestion.

cjcenizal · 2017-06-13T22:37:57Z

ui_framework/components/color_picker/color_picker.js

+  };
+
+  render() {
+    const color = this.props.color || '#ffffff';


I also forgot to mention that I don't think we need this #ffffff here since we have it specified in defaultProps, right?

defaultProps is only used is the prop isn't passed at all, not if what is passed is null or undefined. So for the example:

export class ColorPicker extends React.Component { constructor(props) { super(props); this.state = { color: null }; } handleChange = (value) => { this.setState({ color: value }); }; render() { return <KuiColorPicker onChange={ this.handleChange } color={ this.state.color }/>; } }

it ends up passing null the first time, which means that nothing shows up. So I can either handle this externally, but it seemed easier to handle it internally. What do you think?

Oh I see! Hm. I just read up on what the React team thinks about this, and here's what I'm thinking:

What happens if null is provided? I'd expect either nothing to happen, or an error. In either case, I wouldn't expect null to be interpreted as white. If it's an error, then should we just surface that error instead of hiding it?

I think we can set color: PropTypes.string.isRequired to have it default to white in the case that it's undefined.

Then we can get rid of the check within the render method.

Thoughts on this? It just feels a bit brittle and unexpected to have these two checks in different places.

Okay, I'm happy to use the isRequired trick and expect containers to pass in undefined instead of null in order to default to the components choice of default color. Just didn't want to require the container to either pick a default, or do something fancy to determine whether or not to pass in a prop at all.

That makes sense. Though now that you mention it, I think it makes sense for the owner to pick a default. Since this component has no knowledge of its context, it doesn't really know whether a sensible default would be white, pink, or fuchsia (which, now that I google it, is the same color as pink... go figure).

haha. well I can go either way. So are you saying you want me to remove the defaultProp = white entirely?

Yeah let's just get rid of the defaultProp for now.

zinckiwi · 2017-06-13T22:43:16Z

Yup, that's the gist: catch all clicks except those originating in the popup.

cjcenizal · 2017-06-13T22:43:28Z

ui_framework/components/color_picker/color_picker.js

+        >
+          <div
+            className="kuiColorPicker__swatch"
+            aria-label="Select a color"


I also forgot to mention I love these aria-labels!! Accessibility out of the box.

Can we actually change this to be aria-label={ this.props['aria-label'] } and add a defaultProp for ariaLabel of "Select a color"?

The reason is that if someone provides an aria-label to this component in the current implementation, it will be applied to the root element. Due to the way screen readers interpret this attribute, only that root element's aria-label value will be read aloud, and the other aria-labels that belong to the child elements will be ignored entirely.

We need to change this line to be aria-label={ this.props['aria-label'] } and we need to remove the aria-label that's being set on the root element on line 50.

Whoops, sorry about that, slipped my radar

stacey-gammon · 2017-06-14T14:32:33Z

Interesting, test failures are because of the latest changes, but the element works fine when I ran locally. Will have to investigate.

 FAIL  ui_framework/components/color_picker/color_picker.test.js
  ● pop up color selector is shown when the color swatch is clicked

    TypeError: e.nativeEvent.stopImmediatePropagation is not a function
      
      at KuiColorPicker._this.onClickRootElement (ui_framework/components/color_picker/color_picker.js:30:21)

zinckiwi · 2017-06-14T14:35:43Z

Ah yes. I'm not sure what the setup is on kibana but if you're not running that test through a real browser (phantom, Chrome, or the like) it would make sense there is no nativeEvent. A conditional around trying to access it should suffice.

cjcenizal · 2017-06-14T18:39:55Z

@stacey-gammon I followed up with a couple more comments.

…l fail without a check

stacey-gammon · 2017-06-14T18:51:38Z

ugh, sorry I messed up the history by rebasing. Didn't think it would do that, just wanted to push a change to one of my commits prior to the merge with master.

…-color-picker

stacey-gammon · 2017-06-14T20:58:04Z

test failure:

ERROR: Failed to upload files
com.amazonaws.SdkClientException: Unable to execute HTTP request: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.handleRetryableException(AmazonHttpClient.java:1069)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1035)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:742)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:716)

jenkins, test this

stacey-gammon · 2017-06-19T13:22:57Z

Good to go for a final look @cjcenizal!

cjcenizal

This is badass. Just had one suggestion, then let's merge this!

cjcenizal · 2017-06-19T16:24:06Z

ui_framework/components/color_picker/_color_picker.scss

@@ -0,0 +1,29 @@
+.kuiColorPicker {
+  cursor: pointer;


What do you think of adding display: inline-block here? This way the hit area of the element is restricted to just the color swatch and hex value. With the current display defaulting to block the entire horizontal area to the right of the color picker is clickable, even though it's just white space. Changing it to inline-block would also make it more consistent with form elements like text inputs.

…lab el

…-color-picker

zinckiwi · 2017-06-21T20:31:16Z

(Can't reply inline with the multiple-pickers comment for some reason)

I suppose I had assumed that these pickers were modal -- that is, only one would be displayed at a time. Do they share a common parent element? It's probably a matter of finding the right element in the tree that is unique to each picker.

stacey-gammon · 2017-06-21T20:44:13Z

@zinckiwi I think I have a solution here: #12458

Only one should be displayed at a time, but we aren't forcing them to be modal (e.g. external clicks don't just close the picker but also do whatever the click should do - so you can go from one open color picker to another with a single click).

Add new react color picker to ui framework with tests

33abecb

In prep for dashboard color customization.

stacey-gammon requested a review from cjcenizal June 8, 2017 20:14

snide reviewed Jun 8, 2017

View reviewed changes

cjcenizal suggested changes Jun 9, 2017

View reviewed changes

address code review comments

ea69a13

stacey-gammon added :Sharing review v6.0.0 labels Jun 9, 2017

cjcenizal suggested changes Jun 13, 2017

View reviewed changes

cjcenizal reviewed Jun 13, 2017

View reviewed changes

second round of feedback

340afd6

stacey-gammon added 2 commits June 14, 2017 14:49

nativeEvent not available in a headless browser, so jenkins tests wil…

5855257

…l fail without a check

move aria-label

3eb1246

stacey-gammon force-pushed the react-color-picker branch from faf8715 to ddf760c Compare June 14, 2017 18:49

stacey-gammon force-pushed the react-color-picker branch 5 times, most recently from fea4c4e to 4df0dd7 Compare June 14, 2017 19:22

stacey-gammon added 2 commits June 14, 2017 15:22

Require color and have components pass undefined to use default color

c82ac7c

update snapshots after aria-label move

341452c

stacey-gammon force-pushed the react-color-picker branch from 4df0dd7 to 341452c Compare June 14, 2017 19:22

stacey-gammon added 2 commits June 14, 2017 15:33

get rid of default color

998ca46

Merge branch 'master' of https://github.com/elastic/kibana into react…

c4c1cf8

…-color-picker

cjcenizal approved these changes Jun 19, 2017

View reviewed changes

stacey-gammon added 2 commits June 19, 2017 14:09

user display: inline-block so clickable is only the width of box and …

31d13e0

…lab el

Merge branch 'master' of https://github.com/elastic/kibana into react…

983032c

…-color-picker

stacey-gammon merged commit 4c3cf8b into elastic:master Jun 19, 2017

jimgoodwin added the release_note:enhancement label Jul 25, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

stacey-gammon deleted the react-color-picker branch October 24, 2017 13:55

		@@ -0,0 +1 @@
		export { KuiColorPicker } from './color_picker';

Add new react color picker to ui framework with tests #12245

Add new react color picker to ui framework with tests #12245

Conversation

stacey-gammon commented Jun 8, 2017

snide left a comment

Choose a reason for hiding this comment

snide Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

snide Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Jun 9, 2017 • edited Loading

stacey-gammon commented Jun 13, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinckiwi commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Jun 14, 2017

zinckiwi commented Jun 14, 2017

cjcenizal commented Jun 14, 2017

stacey-gammon commented Jun 14, 2017

stacey-gammon commented Jun 14, 2017

stacey-gammon commented Jun 19, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinckiwi commented Jun 21, 2017

stacey-gammon commented Jun 21, 2017

snide Jun 8, 2017 •

edited

Loading

snide Jun 8, 2017 •

edited

Loading

cjcenizal Jun 9, 2017 •

edited

Loading

stacey-gammon commented Jun 9, 2017 •

edited

Loading

cjcenizal Jun 13, 2017 •

edited

Loading