port k7 popover component over

[stacey-gammon]

Design in K6:

Design in k7:

[cjcenizal]

Nice job porting this over! Looks great overall, just had a couple suggestions.

[cjcenizal]

Can we remove this? It doesn't look like it's being used.

[cjcenizal]

Can we remove this? Looks like it's not in use.

[cjcenizal]

I think this one is leftover from a copy-paste.

[cjcenizal]

Since this pattern is now being used for both Popover and the ColorPicker, I wonder if we should extract this out into a reusable component. @zinckiwi didn't you have this idea originally, something like <KuiClickOutsideToClose>?

I didn't test this, but I think we could generally use it like this:

<KuiClickOutsideToClose
  onClickOutside={this.props.closePopover}
>
  <div
    className={ classes }
    { ...rest }
  >
    {button}
    {body}
  </div>
</KuiClickOutsideToClose>

And I think the component would look like this:

export class KuiClickOutsideToClose extends Component {
  constructor(props) {
    super(props);

    // Use this variable to differentiate between clicks on the element that should not cause the pop up
    // to close, and external clicks that should cause the pop up to close.
    this.didClickMyself = false;
  }

  onClickOutside = () => {
    if (this.didClickMyself) {
      this.didClickMyself = false;
      return;
    }
 
    this.props.onClickOutside();
  };

  onClickRootElement = () => {
    // This prevents clicking on the element from closing it, due to the event handler on the
    // document object.
    this.didClickMyself = true;
  };

  componentDidMount() {
    // When the user clicks somewhere outside of the color picker, we will dismiss it.
    document.addEventListener('click', this.onClickOutside);
  }

  componentWillUnmount() {
    document.removeEventListener('click', this.onClickOutside);
  }

  render() {
    const props = Object.assign({}, this.props.children.props, {
      onClick: this.onClickRootElement,
    });

    return cloneElement(this.props.children, props);
  }
}

Thoughts?

[zinckiwi]

Yeah, that's the gist. It can also be extended in the future to handle tabbing-out of its content if applicable/desirable.

Unless there's a recent change I'm not up to date on, I believe the preferred pattern for the child mutation is:

render() {
  return React.Children.only(child =>
    React.cloneElement(child, {
      onClick: this.onClickRootElement
    })
}

It's a stylistic preference as to whether to enforce a single child (with React.Children.only) or to support arbitrary content (in which case you'll need to render a containing div or equivalent, and attach the onClick handler to that). I have a slight preference for the latter so you don't have to remember that child React components need to handle the onClick, but it does create an extra element.

[cjcenizal]

so you don't have to remember that child React components need to handle the onClick

Could you explain what you mean by this? KuiClickOutsideToClose is adding the onClick handler to the child, so I'm not sure I see why you'd have to remember that.

[stacey-gammon]

I'm in favor, but mind if I create an issue for this to track/tackle it separately and not be a requirement of this PR?

[cjcenizal]

Sure, thanks @stacey-gammon!

[zinckiwi]

It adds it to the child which is fine for html elements (div etc.) since they implicitly bind any html handler. But React components don't have to. I could have a <SomeChild /> component as the child of KuiClickOutsideToClose and if it didn't {...props} or onclick={ this.props.onClick } nothing would happen.

[cjcenizal]

Could we add buttonType="basic" to the Button instances in these examples, just so they don't look funny?

[cjcenizal]

We can match the typography of other components by changing this to:

    line-height: $globalLineHeight;
    font-size: $globalFontSize;

[cjcenizal]

🚀 Awesome! I'm so glad we're able to port components over like this.