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.

Sign up for GitHub

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

Jump to bottom

port k7 popover component over #13322

Merged
merged 5 commits into from
Aug 9, 2017
Merged

port k7 popover component over #13322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7de580
port k7 popover component over
stacey-gammon Aug 3, 2017
35824c8
Fix line height
stacey-gammon Aug 3, 2017
2f029ec
Merge branch 'master' of https://github.com/elastic/kibana into desig…
stacey-gammon Aug 8, 2017
9d2f725
Clean up css
stacey-gammon Aug 8, 2017
b3b0e6f
Merge branch 'master' of https://github.com/elastic/kibana into desig…
stacey-gammon Aug 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ui_framework/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ export {
KuiPagerButtonGroup,
} from './pager';

export {
KuiPopover,
} from './popover';

export {
KuiTabs,
KuiTab
Expand Down
1 change: 1 addition & 0 deletions ui_framework/components/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
@import "notice/index";
@import "pager/index";
@import "panel/index";
@import "popover/index";
@import "empty_table_prompt/index";
@import "status_text/index";
@import "table/index";
Expand Down
82 changes: 82 additions & 0 deletions ui_framework/components/popover/__snapshots__/popover.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`KuiPopover anchorPosition defaults to center 1`] = `
<div
class="kuiPopover"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;

exports[`KuiPopover anchorPosition left is rendered 1`] = `
<div
class="kuiPopover kuiPopover--anchorLeft"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;

exports[`KuiPopover anchorPosition right is rendered 1`] = `
<div
class="kuiPopover kuiPopover--anchorRight"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;

exports[`KuiPopover children is rendered 1`] = `
<div
class="kuiPopover"
>
<button />
<div
class="kuiPopover__body"
>
Children
</div>
</div>
`;

exports[`KuiPopover is rendered 1`] = `
<div
aria-label="aria-label"
class="kuiPopover testClass1 testClass2"
data-test-subj="test subject string"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;

exports[`KuiPopover isOpen defaults to false 1`] = `
<div
class="kuiPopover"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;

exports[`KuiPopover isOpen renders true 1`] = `
<div
class="kuiPopover kuiPopover-isOpen"
>
<button />
<div
class="kuiPopover__body"
/>
</div>
`;
3 changes: 3 additions & 0 deletions ui_framework/components/popover/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
$popOverBackgroundColor: #FFF;

@import 'popover';
95 changes: 95 additions & 0 deletions ui_framework/components/popover/_popover.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Pop menu is an animated popover relatively positioned to a button / action.
// By default it positions in the middle, but can be anchored left and right.

.kuiPopover {
display: inline-block;
position: relative;

// Open state happens on the wrapper and applies to the body.
&.kuiPopover-isOpen {
.kuiPopover__body {
opacity: 1;
visibility: visible;
z-index: 1;
margin-top: 10px;
box-shadow: 0 16px 16px -8px rgba(0, 0, 0, 0.1);
}
}
}

// Animation happens on the body.
.kuiPopover__body {
line-height: $globalLineHeight;
font-size: $globalFontSize;
position: absolute;
min-width: 256px; // Can expand further, but this size is good for our menus.
top: 100%;
left: 50%;
background: $popOverBackgroundColor;
border: 1px solid $globalBorderColor;
border-radius: $globalBorderRadius 0 $globalBorderRadius $globalBorderRadius;
padding: 16px;
transform: translateX(-50%) translateY(8px) translateZ(0);
backface-visibility: hidden;
transform-origin: center top;
opacity: 0;
visibility: hidden;
margin-top: 32px;

// This fakes a border on the arrow.
&:before {
position: absolute;
content: "";
top: -16px;
height: 0;
width: 0;
left: 50%;
margin-left: -16px;
border-left: 16px solid transparent;
border-right: 16px solid transparent;
border-bottom: 16px solid $globalBorderColor;
}

// This part of the arrow matches the body.
&:after {
position: absolute;
content: "";
top: -16px + 1;
right: 0;
height: 0;
left: 50%;
margin-left: -16px;
width: 0;
border-left: 16px solid transparent;
border-right: 16px solid transparent;
border-bottom: 16px solid $popOverBackgroundColor;
}
}


// Positions the menu and arrow to the left of the parent.
.kuiPopover--anchorLeft {
.kuiPopover__body {
left: 0;
transform: translateX(0%) translateY(8px) translateZ(0);

&:before, &:after {
right: auto;
left: 8px;
margin: 0;
}
}
}

// Positions the menu and arrow to the right of the parent.
.kuiPopover--anchorRight {
.kuiPopover__body {
left: 100%;
transform: translateX(-100%) translateY(8px) translateZ(0);

&:before, &:after {
right: 8px;
left: auto;
}
}
}
3 changes: 3 additions & 0 deletions ui_framework/components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export {
KuiPopover,
} from './popover';
100 changes: 100 additions & 0 deletions ui_framework/components/popover/popover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import React, { Component } from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';

const anchorPositionToClassNameMap = {
'center': '',
'left': 'kuiPopover--anchorLeft',
'right': 'kuiPopover--anchorRight',
};

export const ANCHOR_POSITIONS = Object.keys(anchorPositionToClassNameMap);

export class KuiPopover 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;
}

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

this.props.closePopover();
};

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

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

render() {
const {
anchorPosition,
bodyClassName,
button,
isOpen,
children,
className,
closePopover, // eslint-disable-line no-unused-vars
...rest,
} = this.props;

const classes = classNames(
'kuiPopover',
anchorPositionToClassNameMap[anchorPosition],
className,
{
'kuiPopover-isOpen': isOpen,
},
);

const bodyClasses = classNames('kuiPopover__body', bodyClassName);

const body = (
<div className={ bodyClasses }>
{ children }
</div>
);

return (
<div
onClick={ this.onClickRootElement }
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link

@zinckiwi zinckiwi Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, thanks @stacey-gammon!

Copy link

Choose a reason for hiding this comment

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

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.

className={ classes }
{ ...rest }
>
{ button }
{ body }
</div>
);
}
}

KuiPopover.propTypes = {
isOpen: PropTypes.bool,
closePopover: PropTypes.func.isRequired,
button: PropTypes.node.isRequired,
children: PropTypes.node,
anchorPosition: PropTypes.oneOf(ANCHOR_POSITIONS),
bodyClassName: PropTypes.string,
};

KuiPopover.defaultProps = {
isOpen: false,
anchorPosition: 'center',
};
Loading