tooltip component #174

nreese · 2017-11-30T23:47:14Z

Adds tooltip component

Adds new service noOverflowPlacement that contains reusable logic for determining placement (top, right, bottom, left) that avoids clipping by window view port

bevacqua

Left some code review comments trying to keep the coding style consistent with what we have in EUI

bevacqua · 2017-12-01T02:44:09Z

src/components/tooltip/tooltip.js

+ render() {
+ let {isSticky, visible, size, className, children, ...others} = this.props;
+
+ const newClasses = classnames('tooltip-container', visible ? 'tooltip-container-visible' : 'tooltip-container-hidden',


Using this function, you can use an object for clarity when it comes to optional classes:

const newClasses = classnames('tooltip-container', { 'tooltip-container--visible': visible, 'tooltip-container--hidden': !visible, 'tooltip-container--hoverable': isSticky, [`tooltip-${ size }`]: size !== 'auto' }, className)

bevacqua · 2017-12-01T02:44:28Z

src/components/tooltip/tooltip.js

+ }
+
+ render() {
+ let {isSticky, visible, size, className, children, ...others} = this.props;


Can we use const here?

bevacqua · 2017-12-01T02:46:50Z

src/components/tooltip/tooltip.js

+
+ constructor(props) {
+ super(props);
+ this.state = {visible: props.trigger === 'manual' ? props.display : false};


It'd be best to break this up into multiple lines to keep things simple:

const visible = props.trigger === 'manual' ? props.display : false this.state = { visible };

Consider adding a comment about what the conditional does

bevacqua · 2017-12-01T02:48:41Z

src/components/tooltip/tooltip.js

+ return (
+ <div {...newProps}>
+ {this.props.children}
+ <Tooltip {...{isSticky, size: this.props.size, visible}}>{tooltip}</Tooltip>


Move this to a variable for readability:

const tooltipProps = {isSticky, size, visible} return ...<Tooltip {...tooltipProps}>...

bevacqua · 2017-12-01T02:51:23Z

src/components/tooltip/tooltip.js

+ onMouseLeave: this.hoverHandler.bind(this)
+ };
+ break;
+ }


Let's move all of this out of the way and into its own function:

const triggerHandler = getTriggerHandler.call(this) ... function getTriggerHandler() { if (trigger === 'manual') { return {} } if (triggerHandler === 'click') { return { onClick: e => this.clickHandler(e, onClick) } } return { onClick, onMouseEnter: this.hoverHandler.bind(this), onMouseLeave: this.hoverHandler.bind(this) } }

bevacqua · 2017-12-01T02:53:45Z

src/components/tooltip/tooltip.js

+ }
+
+ render() {
+ const {isSticky, placement, tooltip, trigger, className, clickHideDelay, onEntered, onExited, theme, size, onClick, display, ...others} = this.props;


Maybe break this up to list one property per line?

bevacqua · 2017-12-01T02:56:04Z

src/components/tooltip/tooltip.js

+ onClick(e);
+ setTimeout(() => {
+ this.setState({visible: false});
+ }, this.props.clickHideDelay);


What does this component accomplish? It looks like the Focus Trap we're already using elsewhere can do this job without duplicating logic /cc @cjcenizal

bevacqua · 2017-12-01T02:56:47Z

src/components/tooltip/tooltip.js

+export class Tooltip extends React.PureComponent {
+ static propTypes = {
+ visible: PropTypes.bool,
+ size: PropTypes.oneOf(['auto','sm', 'md', 'lg']),


The EUI standard sizing properties are single letters: s, m, l. auto is fine as-is

bevacqua · 2017-12-01T02:57:36Z

src/components/tooltip/tooltip.js

+ static defaultProps = {
+ visible: true,
+ size: 'auto',
+ isSticky: false


I'd argue that we either have property names like isVisible isSticky everywhere or visible, sticky, for consistency

bevacqua · 2017-12-01T02:57:49Z

src/components/tooltip/tooltip.js

+ isSticky: false
+ };
+
+ constructor(props) {


Redundant constructor can be safely removed

snide · 2017-12-01T04:23:24Z

@nreese Cool. We really need this. I will rewrite the CSS layer (and can do a pass for naming consistency of your props as well) for you tomorrow so don't worry about that side. I can make this pretty with some nice quick animations! From a feature perspective there are a few things that we should probably have.

The placement prop should likely accept auto as a value (it should also be the default prop). Basically it would just use top as the value unless the tooltip hit any of the sides of the window. Likely gonna need to do some JS calculations for position. For example, if the tooltip has no room on the right, it should use left as a the placement.
Tooltips should accept both a title and a body prop. Body is required, title is not.

cchaos · 2017-12-01T15:00:37Z

...unless the tooltip hit any of the sides of the window. Likely gonna need to do some JS calculations for position. For example, if the tooltip has no room on the right, it should use left as a the placement.

+1

I noticed the Pivotal's example for the right positioned tooltip not only extends visually past the window but also causes the window to scroll horizontally. It would be nice if ours was smarter than that and ensures that the tooltip is always positioned inside the current window.

cjcenizal

I just have a few minor suggestions but overall this looks like a great start. I think we can merge this to unblock you, and then we can refine the styles and functionality in subsequent PRs.

cjcenizal · 2017-12-04T17:10:07Z

src-docs/src/views/tooltip/tooltip_example.js

+ </p>
+ </div>
+ }
+ demo={


Can we extract this demo code into an separate file, so we can also show the source code for it? (Similar to the docs for the other components).

cjcenizal · 2017-12-04T17:14:55Z

src-docs/src/views/tooltip/tooltip_example.js

+ text={
+ <div>
+ <p>
+ https://styleguide.pivotal.io/tooltips#tooltip-triggers


Can we remove this? I think it's good to keep the link the description of this PR as a reference, but I think we should own this component instead of relying on external documentation for it.

cjcenizal · 2017-12-04T17:16:58Z

src-docs/src/views/tooltip/tooltip_example.js

+
+export default props => (
+ <GuidePage title={props.route.name}>
+ <GuideSection


Can we add a CallOut here with a note that we're going to be changing this around a lot? I just want people to be aware that the Tooltip will change as we define our requirements more clearly.

import { EuiButton, EuiCallOut, EuiCode, EuiSpacer, } from '../../../../src/components'; /* ... */ <GuidePage title={props.route.name}> <EuiCallOut title="Work in progress" color="warning" > <p> This component is still undergoing active development, and its interface and implementation are both subject to change. </p> </EuiCallOut> <EuiSpacer size="l" />

cjcenizal · 2017-12-04T17:22:23Z

src/components/tooltip/tooltip.js

+
+ return (
+ <div className={newClasses} {...others}>
+ <div className="tooltip-content">{children}</div>


Re @snide's comment about body and title, I think we should stick with using children in lieu of body personally -- this feels more ergonomic to me. We've been trying to use children wherever possible. And this way we can add the title prop later without needing to change anything else about the interface.

cjcenizal · 2017-12-04T17:24:43Z

@nreese Looks like the CI is failing due to linting errors. You can run npm run lint and npm run lint-fix. We have a precommit hook which will also run these for you.

cjcenizal

I just merged some changes into master which will cause merge conflicts. Here are the steps for resolving them.

cjcenizal · 2017-12-04T19:01:28Z

src-docs/src/services/routes/routes.js

@@ -101,6 +101,9 @@ import TextExample
 import ToastExample
 from '../../views/toast/toast_example';

+import TooltipExample


To fix the merge conflicts with master, this will have to become:

import { TooltipExample } from '../../views/tooltip/tooltip_example';

cjcenizal · 2017-12-04T19:02:04Z

src-docs/src/services/routes/routes.js

@@ -212,6 +215,9 @@ const components = [{
 }, {
 name: 'Toast',
 component: ToastExample,
+}, {
+ name: 'Tooltip',
+ component: TooltipExample,


This array of objects has become just a single-dimensional array of references to the example files, so you can replace this object with just TooltipExample.

cjcenizal · 2017-12-04T19:02:27Z

src-docs/src/views/tooltip/tooltip_example.js

+ }
+ />
+ </GuidePage>
+);


This entire example will become:

import React from 'react'; import { renderToHtml } from '../../services'; import { GuidePage, GuideSection, GuideSectionTypes, } from '../../components'; import { EuiCallOut, EuiSpacer, EuiButton, EuiCode, } from '../../../../src/components'; import TooltipExamples from './examples'; const examplesSource = require('!!raw-loader!./examples'); const examplesHtml = renderToHtml(TooltipExamples); export const TooltipExample = { title: 'Tooltip', intro: ( <div> <EuiCallOut title="Work in progress" color="warning" > <p> This component is still undergoing active development, and its interface and implementation are both subject to change. </p> </EuiCallOut> <EuiSpacer size="l" /> </div> ), sections: [{ title: 'Tooltip', source: [{ type: GuideSectionTypes.JS, code: '', }, { type: GuideSectionTypes.HTML, code: '', }], text: ( <p> </p> ), demo: <TooltipExamples />, }], };

cjcenizal · 2017-12-04T19:04:08Z

src-docs/src/views/tooltip/examples.js

+ <div>
+ <div>
+ Check out this
+ <TooltipTrigger tooltip="I am the body" title="I am the title">


Due to the way whitespace is interpreted, there is no space between "this" and "tooltip" in these examples:

We can fix this like this:

Check out this {( <TooltipTrigger tooltip="I am the body" title="I am the title"> <span className="overlay-trigger" tabIndex="0">tooltip with title.</span> </TooltipTrigger> )}

nreese · 2017-12-05T15:37:15Z

@cchaos Are you planning on doing the CSS work against this PR or should I finish getting this PR reviewed and merged and then have the CSS work done against master?

cchaos · 2017-12-05T16:20:48Z

@nreese Yeah I think that is the plan.

nreese · 2017-12-05T16:50:16Z

@bevacqua @snide I have moved the PR out of the WIP state and am ready for reviews

snide

I'm ok with this for now given the warning at the top. @cchaos and I will work to get the styling done and likely cleanup some of the props / interface so that it's a little simpler, closer to our spec. Functionally though it does everything we need it to do for design to get involved. Thanks @nreese.

bevacqua suggested changes Dec 1, 2017

View reviewed changes

cjcenizal mentioned this pull request Dec 1, 2017

EUIfying Kibana elastic/kibana#15282

Closed

9 tasks

cjcenizal approved these changes Dec 4, 2017

View reviewed changes

cjcenizal reviewed Dec 4, 2017

View reviewed changes

nreese added 6 commits December 4, 2017 13:09

tooltip from pivotal-ui

17d9011

updates from bevacqua review

37879bf

add title property

1d48ff7

update demo with warning and show source + html

7bdc0e9

put title in tooltip-content div

bd0e937

do not allow tooltip to overflow window bounds

851098a

nreese force-pushed the tooltip branch from ca1def9 to 851098a Compare December 4, 2017 20:09

nreese added 2 commits December 4, 2017 13:16

add space to demos

5739de2

fix lint errors

ef361b3

snide mentioned this pull request Dec 4, 2017

Near term EUI roadmap #108

Closed

51 tasks

check cross dimension for tooltip placement

1d3d267

nreese changed the title ~~[WIP] tooltip component~~ tooltip component Dec 5, 2017

rename cross dimension overflow function for better readability

3a5deb0

ensure tooltip opened by click does not get clipped by window viewport

ce54185

move overflow logic to service

f1e63e0

nreese requested a review from snide December 5, 2017 16:50

snide approved these changes Dec 5, 2017

View reviewed changes

nreese merged commit c970d73 into elastic:master Dec 6, 2017

cjcenizal mentioned this pull request Dec 6, 2017

[UI Framework] Add KuiTooltip component and noOverflowPlacement service. elastic/kibana#15460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tooltip component #174

tooltip component #174

nreese commented Nov 30, 2017 •

edited

Loading

bevacqua left a comment

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

bevacqua Dec 1, 2017

snide commented Dec 1, 2017

cchaos commented Dec 1, 2017

cjcenizal left a comment

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017

cjcenizal commented Dec 4, 2017

cjcenizal left a comment

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017

cjcenizal Dec 4, 2017 •

edited

Loading

nreese commented Dec 5, 2017

cchaos commented Dec 5, 2017

nreese commented Dec 5, 2017

snide left a comment

tooltip component #174

tooltip component #174

Conversation

nreese commented Nov 30, 2017 • edited Loading

bevacqua 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

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

snide commented Dec 1, 2017

cchaos commented Dec 1, 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 commented Dec 4, 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

cjcenizal Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

nreese commented Dec 5, 2017

cchaos commented Dec 5, 2017

nreese commented Dec 5, 2017

snide left a comment

Choose a reason for hiding this comment

nreese commented Nov 30, 2017 •

edited

Loading

cjcenizal Dec 4, 2017 •

edited

Loading