[Labs] Tooltip 2 #1438

llorca · 2017-08-11T20:27:08Z

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

Tooltip 2.0 based on Popover 2.0.

isModal -> hasBackdrop

blueprint-bot · 2017-08-11T20:42:59Z

Fix Popover 2 tests

Preview: documentation
Coverage: core | datetime

adidahiya

uncontrolled mode isn't working yet

that's strange; are you sure Popover2's uncontrolled mode works?

adidahiya · 2017-08-11T20:44:09Z

packages/labs/src/components/tooltip/tooltip2.tsx

+    };
+
+    public render() {
+        const { children, intent, tooltipClassName } = this.props;


missing openOnTargetFocus

Hmm, we're spreading the rest props already, so that should be enough? I also added a defaultProps field for it

Actually according to @giladgray we don't even need it in defaultProps because it's already in Popover2 and unchanged here

you could do the following to avoid passing unsupported props?

const { children, intent, tooltipClassName, ...restProps } = this.props; <Tooltip2 {...restProps} ... />

llorca · 2017-08-11T20:47:44Z

Yup Popover2's uncontrolled mode seems to be working just fine: https://3594-71939872-gh.circle-artifacts.com/0/home/ubuntu/blueprint/packages/site-docs/dist/index.html#labs.popover-2.0 (click Hover (target only))

giladgray

Great work @llorca! A few loose ends.

giladgray · 2017-08-11T20:42:21Z

packages/labs/src/components/popover/popover2.tsx

+     * If false, it is attached to a new element appended to `<body>`.
+     * @default false
+     */
+    inline?: boolean;



Why add this? It comes from the extends to share docs with other component

Which extends exactly? I didn't see any so I was super confused

extends IOverlayableProps on the interface itself, line 30. please remove this prop.

@llorca yep, this prop is already available in the props interface due to the fact that IPopoverProps2 extends IOverlayableProps. Note in overlay.tsx#L49 that IOverlayableProps already includes inline?: boolean, so we should delete this re-definition here.

giladgray · 2017-08-11T20:43:23Z

packages/labs/src/components/popover/popover2.tsx

@@ -255,12 +262,12 @@ export class Popover2 extends AbstractComponent<IPopover2Props, IPopover2State>
        const target = React.cloneElement(children.target,
            // force disable single Tooltip child when popover is open (BLUEPRINT-552)
            (isOpen && children.target.type === Tooltip)


Check for Tooltip2 here, disabled doesn't exist on original Tooltip

giladgray · 2017-08-11T20:45:23Z

packages/labs/src/components/tooltip/tooltip2.tsx

+        openOnTargetFocus: true,
+        placement: "auto",
+        rootElementTag: "span",
+        transitionDuration: 100,


Please only set defaults for props that have different defaults than Popover2. If they're the same then it's best to only set default in one place.

Ok, will remove. Was confused cause I didn't see where Popover2 extends the tooltip stuff.

giladgray · 2017-08-11T20:46:15Z

packages/labs/src/labs.md

@@ -165,6 +165,8 @@ This interface is generic, accepting a type parameter `<T>` for an item in the l
 - ...except for the handful of Tether-specific props, which are now Popper.js-specific:
  - 🔥 `position: Position` &rarr; `placement: PopperJS.Placement`
  - 🔥 `tetherOptions: ITetherOptions` &rarr; `modifiers: PopperJS.Modifiers`
+  - 🔥 `isModal` &rarr; `hasBackdrop`
+  - 🔥 `isDisabled` &rarr; `disabled`


Doesn't fit with the description of Tether-specific

giladgray · 2017-08-11T20:47:08Z

packages/labs/src/labs.md

@@ -223,3 +225,9 @@ Modifiers are the tool through which you customize Popper.js's behavior. It defi
 <div class="pt-callout pt-intent-primary pt-icon-info-sign">
    To understand all the Popper.js modifiers available to you, you'll want to read [the Popper.js Modifiers documentation](https://popper.js.org/popper-documentation.html#modifiers).
 </div>
+
+@## Tooltip 2.0


Any docs? Anything? At least explain it's a thin wrapper to add css class and change some defaults

I'll add docs later, I want to get it working first

giladgray · 2017-08-11T20:47:56Z

packages/site-docs/src/styles/_examples.scss

@@ -199,6 +199,12 @@
  }
 }

+.docs-tooltip2-example {
+  > div {
+    margin-bottom: $pt-grid-size;


Could you put things in <p> tags instead which already have margin bottom?

That's what it was before, had to change it because I was getting this error:

Warning: validateDOMnesting(...): <div> cannot appear as a descendant of <p>. See ... SomeComponent > p > ... > SomeOtherComponent > ReactTooltip > div.

oh sure that old thing

llorca · 2017-08-11T21:36:19Z

packages/labs/src/components/popover/popover2.tsx

@@ -260,7 +260,7 @@ export class Popover2 extends AbstractComponent<IPopover2Props, IPopover2State>

        const children = this.understandChildren();
        const targetTabIndex = this.props.openOnTargetFocus && this.isHoverInteractionKind() ? 0 : undefined;
-        const target = React.cloneElement(children.target,
+        const target: JSX.Element = React.cloneElement(children.target,


This is necessary, otherwise we get:

error TS7022: 'target' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.

Error started showing up after changing Tooltip to Tooltip2 below

blueprint-bot · 2017-08-11T21:41:59Z

Add type to target variable

Preview: documentation
Coverage: core | datetime

…ooltip2

blueprint-bot · 2017-08-11T22:05:29Z

Merge branch 'al/tooltip2' of github.com:palantir/blueprint into al/tooltip2

Preview: documentation
Coverage: core | datetime

giladgray · 2017-08-11T23:53:52Z

packages/labs/src/components/tooltip/tooltip2.tsx

+    public static defaultProps: Partial<ITooltip2Props> = {
+        hoverCloseDelay: 0,
+        hoverOpenDelay: 100,
+        isOpen: false,


❌ remove this default, that's why uncontrolled mode doesn't work

giladgray · 2017-08-11T23:55:00Z

packages/labs/src/labs.md

- ...except for the handful of Tether-specific props, which are now Popper.js-specific:
+- all the classic `Popover` features are still supported, with the same names except...
+    - 🔥 `isModal` &rarr; `hasBackdrop`
+    - 🔥 `isDisabled` &rarr; `disabled`


inconsistent indentation, use two spaces here for minimal diff

giladgray · 2017-08-11T23:56:08Z

packages/labs/src/components/tooltip/tooltip2.tsx

+    /**
+     * Whether or not the tooltip is visible. Passing this property will put the tooltip in
+     * controlled mode, where the only way to change visibility is by updating this property.
+     * @default undefined


remove this line, we don't declare undefined defaults

giladgray · 2017-08-11T23:56:50Z

packages/labs/src/components/tooltip/tooltip2.tsx

+
+    /**
+     * Whether or not the tooltip is visible. Passing this property will put the tooltip in
+     * controlled mode, where the only way to change visibility is by updating this property.


"property" -> "prop"

giladgray · 2017-08-11T23:58:46Z

packages/labs/src/components/tooltip/tooltip2.tsx

+    };
+
+    public render() {
+        const { children, intent, tooltipClassName } = this.props;


you could do the following to avoid passing unsupported props?

const { children, intent, tooltipClassName, ...restProps } = this.props; <Tooltip2 {...restProps} ... />

giladgray · 2017-08-11T23:58:48Z

packages/labs/src/components/tooltip/tooltip2.tsx

+        hoverCloseDelay: 0,
+        hoverOpenDelay: 100,
+        isOpen: false,
+        openOnTargetFocus: true,


cmslewis · 2017-08-12T00:55:00Z

packages/labs/src/components/popover/popover2.tsx

                : { tabIndex: targetTabIndex },
        );

        const isContentEmpty = (children.content == null);
-        if (isContentEmpty && !this.props.isDisabled && isOpen !== false && !Utils.isNodeEnv("production")) {
+        if (isContentEmpty && !this.props.disabled && isOpen !== false && !Utils.isNodeEnv("production")) {
            console.warn("[Blueprint] Disabling <Popover> with empty/whitespace content...");


Popover2?

cmslewis · 2017-08-12T00:56:57Z

packages/labs/src/components/tooltip/tooltip2.tsx

+
+    public static defaultProps: Partial<ITooltip2Props> = {
+        hoverCloseDelay: 0,
+        hoverOpenDelay: 100,


Can we pull this out into a tooltipConstants.ts file if it's the same as the hoverOpenDelay in Tooltip?

Hmm, why? This is used only for the tooltip component, so we'd have to delete that file anyway when we kill Tooltip and rename Tooltip2 to Tooltip in 2.0

Just to ensure they behave identically / to not duplicate subtle values in two places. If we're sure not to export said tooltipConstants.ts file, should be fine.

cmslewis · 2017-08-12T00:57:20Z

packages/labs/src/components/tooltip/tooltip2.tsx

+        hoverOpenDelay: 100,
+        isOpen: false,
+        openOnTargetFocus: true,
+        transitionDuration: 100,


blueprint-bot · 2017-08-14T20:07:07Z

Uncontrolled mode now working

Preview: documentation
Coverage: core | datetime

cmslewis

Code looking good! Left questions. Now for the docs!

cmslewis · 2017-08-14T21:13:47Z

packages/labs/examples/popover2Example.tsx

    inheritDarkTheme?: boolean;
    inline?: boolean;
    interactionKind?: PopoverInteractionKind;
-    isModal?: boolean;


Motivation for hasBackdrop vs isModal?

Mostly consistency with Overlay. Happy to change hasBackdrop to something better later

cmslewis · 2017-08-14T21:16:06Z

packages/labs/src/components/popover/popover2.tsx

+     * If false, it is attached to a new element appended to `<body>`.
+     * @default false
+     */
+    inline?: boolean;



@llorca yep, this prop is already available in the props interface due to the fact that IPopoverProps2 extends IOverlayableProps. Note in overlay.tsx#L49 that IOverlayableProps already includes inline?: boolean, so we should delete this re-definition here.

cmslewis · 2017-08-14T21:17:30Z

packages/labs/src/components/popover/popover2.tsx

@@ -85,7 +93,7 @@ export interface IPopover2Props extends IOverlayableProps, IProps {
     * When modal popovers are opened, they become focused.
     * @default false
     */
-    isModal?: boolean;
+    hasBackdrop?: boolean;


Does the description need to change if we're changing the prop name? (e.g. is it worth using the word "modal" anymore?)

cmslewis · 2017-08-14T21:20:37Z

packages/labs/src/components/popover/popover2.tsx

-            (isOpen && children.target.type === Tooltip)
-                ? { isDisabled: true, tabIndex: targetTabIndex }
+            (isOpen && children.target.type === Tooltip2)
+                ? { disabled: true, tabIndex: targetTabIndex }
                : { tabIndex: targetTabIndex },


nit: maybe a little more concise like this?

const target: JSX.Element = React.cloneElement(children.target, { // force disable single Tooltip child when popover is open (BLUEPRINT-552) disabled: isOpen && children.target.type === Tooltip2 ? true : this.props.disabled, tabIndex: targetTagIndex, });

Fine as is though.

cmslewis · 2017-08-14T21:24:09Z

packages/labs/src/components/tooltip/tooltip2.tsx

+
+    public static defaultProps: Partial<ITooltip2Props> = {
+        hoverCloseDelay: 0,
+        hoverOpenDelay: 100,


Just to ensure they behave identically / to not duplicate subtle values in two places. If we're sure not to export said tooltipConstants.ts file, should be fine.

cmslewis · 2017-08-14T21:24:18Z

packages/labs/src/components/tooltip/tooltip2.tsx

+     * The content that will be displayed inside of the tooltip.
+     */
+    content: JSX.Element | string;
+


Do we still want defaultIsOpen? And disabled?

cmslewis · 2017-08-14T21:24:36Z

packages/labs/src/components/tooltip/tooltip2.tsx

+     * @default 100
+     */
+    hoverOpenDelay?: number;
+


Do we still want inheritDarkTheme, inline, or (for controlled mode) isOpen?

cmslewis · 2017-08-14T21:27:30Z

packages/labs/src/components/tooltip/tooltip2.tsx

+     * portal which holds the tooltip if `inline` is set to `false`.
+     */
+    portalClassName?: string;
+


Do we still want rootElementTag?

llorca · 2017-08-14T23:00:27Z

Sorry about the confusion around which props to include or not, should be good to go now!

blueprint-bot · 2017-08-14T23:06:32Z

Add more props

Preview: documentation
Coverage: core | datetime

blueprint-bot · 2017-08-14T23:43:21Z

Add some docs

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-08-14T23:52:38Z

packages/labs/src/labs.md

@@ -228,6 +228,8 @@ Modifiers are the tool through which you customize Popper.js's behavior. It defi

 @## Tooltip 2.0

+`Tooltip2` uses [`Popover2`](#labs.popover-2.0) under the hood. The public API is the same as [`Tooltip`](#core/components/tooltip), though note that the `isDisabled` prop has been renamed to `disabled`. Read


Can you kick text to a few different lines to achieve shorter line length in the markdown file?

There's a trailing word, Read, that seems superfluous.

cmslewis · 2017-08-14T23:54:58Z

@llorca Needs tests, but everything else looks fine to me!

EDIT: Apparently we don't need to write tests for labs components.

blueprint-bot · 2017-08-15T00:16:12Z

Remove unnecessary word

Preview: documentation | table
Coverage: core | datetime

I believe all comments have been addressed.

Antoine Llorca added 3 commits August 11, 2017 13:24

isDisabled -> disabled

03c9d81

isModal -> hasBackdrop

Initial Tooltip 2 implementation

fe1c971

Fix Popover 2 tests

75878b7

adidahiya reviewed Aug 11, 2017

View reviewed changes

giladgray suggested changes Aug 11, 2017

View reviewed changes

Antoine Llorca added 4 commits August 11, 2017 13:54

Move docs

c9b6ae7

Remove unnecessary props

06be8de

Remove unnecessary import

1b44b0e

Add type to target variable

138e05b

llorca commented Aug 11, 2017

View reviewed changes

Antoine Llorca added 2 commits August 11, 2017 14:58

Add type to target variable

d668948

Merge branch 'al/tooltip2' of github.com:palantir/blueprint into al/t…

c971c01

…ooltip2

giladgray previously requested changes Aug 11, 2017

View reviewed changes

giladgray reviewed Aug 11, 2017

View reviewed changes

cmslewis reviewed Aug 12, 2017

View reviewed changes

Uncontrolled mode now working

8b83264

cmslewis suggested changes Aug 14, 2017

View reviewed changes

Add more props

f2b339e

Add some docs

503de50

cmslewis suggested changes Aug 14, 2017

View reviewed changes

Remove unnecessary word

ed4bc16

cmslewis approved these changes Aug 15, 2017

View reviewed changes

cmslewis merged commit 735d555 into master Aug 15, 2017

cmslewis deleted the al/tooltip2 branch August 15, 2017 00:36

This was referenced Aug 15, 2017

Prepare release v1.25.0 #1448

Merged

[Tooltip2] Fix disabled tooltip #1450

Merged

		@@ -228,6 +228,8 @@ Modifiers are the tool through which you customize Popper.js's behavior. It defi

		@## Tooltip 2.0

		`Tooltip2` uses [`Popover2`](#labs.popover-2.0) under the hood. The public API is the same as [`Tooltip`](#core/components/tooltip), though note that the `isDisabled` prop has been renamed to `disabled`. Read

[Labs] Tooltip 2 #1438

[Labs] Tooltip 2 #1438

Conversation

llorca commented Aug 11, 2017 • edited

Checklist

Changes proposed in this pull request:

blueprint-bot commented Aug 11, 2017

Fix Popover 2 tests

adidahiya 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

llorca commented Aug 11, 2017 • edited

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray Aug 11, 2017 • edited

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

blueprint-bot commented Aug 11, 2017

Add type to target variable

blueprint-bot commented Aug 11, 2017

Merge branch 'al/tooltip2' of github.com:palantir/blueprint into al/tooltip2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray Aug 11, 2017 • edited

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

blueprint-bot commented Aug 14, 2017

Uncontrolled mode now working

cmslewis 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

llorca commented Aug 14, 2017

blueprint-bot commented Aug 14, 2017

Add more props

blueprint-bot commented Aug 14, 2017

Add some docs

Choose a reason for hiding this comment

cmslewis commented Aug 14, 2017 • edited

blueprint-bot commented Aug 15, 2017

Remove unnecessary word

llorca commented Aug 11, 2017 •

edited

llorca commented Aug 11, 2017 •

edited

giladgray Aug 11, 2017 •

edited

giladgray Aug 11, 2017 •

edited

cmslewis commented Aug 14, 2017 •

edited