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

use $globalBorderRadius here.

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";

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);

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;
}

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>
    );
  }

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.

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.

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?

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?

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.

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

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.

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

We can remove this class based on my other suggestion.

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.

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

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.