Graduate Labs Components - Part 2: Replace Popover/Tooltip with Popover2/Tooltip2 #1891

cmslewis · 2017-12-12T01:01:25Z

Delete Tooltip and Popover from core/
Rename Popover2 and Tooltip2 to Popover and Tooltip, respectively
Delete tether dependency
Fix build failures.
Migrate deprecated internal usages to new props (isDisabled, useModal, hasBackdrop)

Note: Context Menu is super screwy now; its calculated transform offset is totally wrong. Can I get some help with the popper modifiers for context menus? Not sure I changed them properly.

Next steps (each in a separate PR, probably):

~~Migrate deprecated internal usages to new props (e.g. Popover2.isDisabled).~~ (done in this PR after all)
Write tests for Tooltip2, Popover, TagInput, and remove them from karma coverageExcludes; bring back old documentation that is still relevant.

giladgray · 2017-12-12T01:02:01Z

packages/core/src/common/errors.ts

@@ -35,6 +35,7 @@ export const NUMERIC_INPUT_STEP_SIZE_NON_POSITIVE =
    ns + ` <NumericInput> requires stepSize to be strictly greater than zero.`;
 export const NUMERIC_INPUT_STEP_SIZE_NULL = ns + ` <NumericInput> requires stepSize to be defined.`;

+// TODO (clewis): Delete old Popover errors that aren't used anymore.


can we get this resolved before merging?

@giladgray Turns out Popover2 doesn't use any of these. Seems like it should still handle these cases, so I'd be happy to leave these and file a follow-up issue. Thoughts?

giladgray · 2017-12-12T01:02:50Z

packages/core/src/components/popover/popover.md

@@ -1,281 +1,73 @@
-@# Popovers


oh there's some very useful stuff in here that we don't want to delete.

Yep, adding it back in Part 4.

wait what's part 3? is it possible to just not delete it now?

giladgray · 2017-12-12T01:04:02Z

packages/core/src/components/popover/popover.tsx

+    /**
+     * Prevents the popover from appearing when `true`.
+     * @deprecated use `disabled`
+     */
    isDisabled?: boolean;


lets just jump straight to the non-deprecated props

adidahiya

going to block on #1885

adidahiya · 2017-12-14T01:39:55Z

packages/core/karma.conf.js

@@ -17,9 +17,9 @@ module.exports = function (config) {
            "src/components/popover/*",
            "src/components/tabs/*",
            // TODO (clewis): write tests for these component as part of the 2.0 effort:
-            "src/components/popover2/*",
+            "src/components/popover/*",


this is now a duplicate of line 17

giladgray · 2017-12-14T21:08:20Z

packages/core/src/components/popover/popover.md


 @reactExample PopoverExample

-@## JavaScript API


I think we likely want to keep this entire section, and all its subsections, as they document key features of Popover that are still relevant.

…u styles

…mal className

blueprint-bot · 2017-12-18T22:47:34Z

Add 'packages/*/node_modules/' to save_cache dirs

Preview: documentation | table

blueprint-bot · 2017-12-18T22:53:11Z

Merge branch 'master' into cl/graduate-labs-to-core-2

Preview: documentation | table

blueprint-bot · 2017-12-18T23:17:20Z

Cache bust: v5 => v6

Preview: documentation | table

adidahiya · 2017-12-18T23:35:35Z

packages/karma-build-scripts/createKarmaConfig.js

@@ -70,7 +70,7 @@ module.exports = function createKarmaConfig({ dirname, coverageExcludes, coverag
            [path.join(dirname, "test/index.ts")]: "webpack",
        },
        reporters: ["mocha", "coverage"],
-        singleRun: true,
+        singleRun: false,


questionable edit... this is probably why the tests are hanging

blueprint-bot · 2017-12-18T23:41:59Z

Undo singlRun flag change

Preview: documentation | table

blueprint-bot · 2017-12-18T23:49:58Z

Revert changes to Circle config file

Preview: documentation | table

blueprint-bot · 2017-12-19T00:09:22Z

Fix table tests

Preview: documentation | table

cmslewis · 2017-12-19T00:15:51Z

@adidahiya @giladgray ready for actual CR now.

cmslewis

Self-review.

cmslewis · 2017-12-19T00:19:03Z

packages/core/src/common/errors.ts

@@ -35,6 +35,7 @@ export const NUMERIC_INPUT_STEP_SIZE_NON_POSITIVE =
    ns + ` <NumericInput> requires stepSize to be strictly greater than zero.`;
 export const NUMERIC_INPUT_STEP_SIZE_NULL = ns + ` <NumericInput> requires stepSize to be defined.`;

+// TODO (clewis): Delete old Popover errors that aren't used anymore.


@giladgray Turns out Popover2 doesn't use any of these. Seems like it should still handle these cases, so I'd be happy to leave these and file a follow-up issue. Thoughts?

cmslewis · 2017-12-19T00:19:51Z

packages/core/src/components/context-menu/contextMenu.tsx

-const TETHER_OPTIONS = {
-    constraints: [{ attachment: "together", pin: true, to: "window" }],
+const POPPER_MODIFIERS: PopperModifiers = {
+    preventOverflow: { boundariesElement: "window" },


Are these settings more or less equivalent to the old constraints?

cmslewis · 2017-12-19T00:22:30Z

packages/core/src/components/index.ts

 export * from "./portal/portal";
 export * from "./progress/progressBar";
-export * from "./tooltip/svgTooltip";


@adidahiya @giladgray React 16 was causing SVGTooltip and SVGPopover tests to fail, because they were trying to render <g> tags outside of an <svg>. Since these components are such thin wrappers around the core Tooltip and Popover, respectively, I opted just to delete them and consider adding documentation about working with these components in SVG land.

Does that sound okay?

code search reveals a very small number (~5) of SVGPopover usages, and it's possible some of those are stale. I hate to say it, but I think we'll want to at least preserve support for this use case, which may require some work to be able to customize elements.

cmslewis · 2017-12-19T00:25:32Z

packages/core/src/components/menu/menuItem.tsx

                    {...popoverProps}
-                    content={submenuElement}
+                    content={submenuRef}
+                    minimal={true}


Use the minimal prop instead of applying the pt-minimal class alone. This ensures the menu won't leave room for an arrow that doesn't even exist.

cmslewis · 2017-12-19T00:26:17Z

packages/core/src/components/menu/menuItem.tsx

-            const parentWidth = this.liElement.parentElement.getBoundingClientRect().width;
-            const adjustmentWidth = submenuRect.width + parentWidth;
+    private maybeAlignSubmenuLeft() {
+        if (this.popoverRef == null) {


Small style improvement: put this check on top and de-intent everything below.

cmslewis · 2017-12-19T00:27:27Z

packages/core/src/components/tooltip/tooltip.md

@@ -1,77 +1,7 @@
-@# Tooltips
+@# Tooltip


Will audit these docs and re-include relevant info in a follow-up PR.

cmslewis · 2017-12-19T00:27:36Z

packages/core/src/components/tooltip/tooltip.tsx

@@ -1,17 +1,16 @@
 /*
- * Copyright 2015 Palantir Technologies, Inc. All rights reserved.
+ * Copyright 2017 Palantir Technologies, Inc. All rights reserved.


2015 because it's the same file in the diff? 2017 because that's when we wrote Tooltip2?

yeah lol I'm not sure! probably 2015 cuz it's the same file.

giladgray · 2017-12-19T00:44:18Z

packages/core/src/components/menu/menuItem.tsx

+        if (this.props.useSmartPositioning) {
+            // Popper.js renders the popover in the DOM before relocating its
+            // position on the next tick. We need to rAF to wait for that to happen.
+            requestAnimationFrame(() => this.maybeAlignSubmenuLeft());


🤔 Popper.js should be powerful enough to handle all the positioning logic: we should let it decide when to flip to the left, because it likely knows better than us. Happy for this to be a follow-up, though.

giladgray · 2017-12-19T00:44:50Z

packages/core/src/components/popover/popover.md

@@ -1,281 +1,73 @@
-@# Popovers


wait what's part 3? is it possible to just not delete it now?

giladgray · 2017-12-19T00:45:16Z

packages/core/src/components/tooltip/tooltip.tsx

@@ -1,17 +1,16 @@
 /*
- * Copyright 2015 Palantir Technologies, Inc. All rights reserved.
+ * Copyright 2017 Palantir Technologies, Inc. All rights reserved.


yeah lol I'm not sure! probably 2015 cuz it's the same file.

giladgray · 2017-12-19T00:45:30Z

packages/core/src/components/tooltip/tooltip.tsx

-     * See http://github.hubspot.com/tether/#constraints
-     * @deprecated since v1.12.0; use `tetherOptions` instead.
+     * Initial opened state when uncontrolled.
+     * @default false


remove this, it defaults to undefined

Doesn't it default explicitly to false below?

https://github.com/palantir/blueprint/pull/1891/files/08312ffb58f969cfa74dc3738982393d142cdc07#diff-0f4e5f29db46149ff7a16aa9869d0ff9R130

giladgray · 2017-12-19T00:46:15Z

packages/core/test/menu/menuTests.tsx

-                </MenuItem>,
-                childContainer,
-            ) as MenuItem;
+                </React.Fragment>,


✨ 😱 ✨

giladgray · 2017-12-19T00:46:42Z

packages/core/test/menu/menuTests.tsx

@@ -293,6 +290,15 @@ describe("MenuItem", () => {
        function assertClassNameCount(className: string, count: number) {
            assert.strictEqual(childContainer.queryAll(`.${className}`).length, count, `${count}x .${className}`);
        }
+
+        function mountMenuItem(props?: IMenuItemProps, childItems?: React.ReactNode) {
+            return ReactDOM.render(


awkward we're not using enzyme here

@giladgray Looks like we need real DOM to properly evaluate all the real-estate-dependent dynamic-layout behavior. Also looks like you were the original test author, so feel free to tell me if that's not the case!

blueprint-bot · 2017-12-19T01:53:43Z

Fix build (was due to errant find+replace), fix comment

Preview: documentation | table

adidahiya

mostly looks good. I know this PR is getting long, but overall this feature isn't at the quality bar to merge into master. I think we should start a feature branch so we can push fixes in follow-up PRs. are you tracking all the remaining tasks in a list somewhere? we might want to make a new issue for that.

noticed this regression on the tooltip example:

adidahiya · 2017-12-19T05:44:54Z

packages/core/test/popover/popoverTests.tsx

@@ -33,13 +46,13 @@ describe("<Popover>", () => {
        testsContainerElement.remove();
    });

-    describe("validation:", () => {
+    describe.skip("validation:", () => {


use { expectPropValidationError } from "@blueprintjs/test-commons";
fine to do in a follow-up PR, just make a note in this PR description

adidahiya · 2017-12-19T05:46:52Z

packages/core/test/popover/popoverTests.tsx

-    it("componentDOMChange does not update targetHeight/targetWidth state when useSmartArrowPositioning=false", () => {
-        const root = renderPopover({
-            useSmartArrowPositioning: false,
+    describe("deprecated prop shims", () => {


should remove this describe block in a follow-up PR

Addressed immediate comments

blueprint-bot · 2017-12-19T19:31:18Z

Fix inline popover targets (CSS)

Preview: documentation | table

cmslewis · 2017-12-19T19:34:22Z

Unrelated test flake failed the build. I'll run it one more time and then just merge into my feature branch if it flakes again (I have a proposed fix for the flaky test btw; will open a new PR if it's foolproof).

cmslewis · 2017-12-19T19:52:05Z

Flaked again. Opened this PR to (hopefully) fix the flaky Overlay test: #1943. Merging this for now.

cmslewis added 11 commits December 11, 2017 16:53

Delete popover/ and tooltip/ (except SVG variants); rename ___2/ to ___/

874c971

Rename 'POPOVER2' => 'POPOVER'

0f68d65

Rename 'popover2' => 'popover'

04ef3b8

Move PopoverInteractionKind into new popover.tsx

2a818e5

Rename 'Popover2' => 'Popover'

408ea31

Fix a bunch more 'Popover2' => 'Popover' renames

a1c47bf

Rename 'tooltip2' => 'tooltip'

4a688e3

Rename 'Tooltip2' => 'Tooltip'

705e10f

Resolve popover2/tooltip2 filenames

09297f7

Whoops, add popover and tooltip CSS files again

a99836a

Fix compile errors

ee150eb

cmslewis requested review from giladgray and adidahiya December 12, 2017 01:04

giladgray reviewed Dec 12, 2017

View reviewed changes

Fix dist:apps via some isDisabled=>disabled references

671d555

adidahiya previously requested changes Dec 12, 2017

View reviewed changes

Merge branch 'master' into cl/graduate-labs-to-core-2

5378c83

adidahiya reviewed Dec 14, 2017

View reviewed changes

Set tag = false in <Manager>

04dc6b6

giladgray reviewed Dec 14, 2017

View reviewed changes

cmslewis added 10 commits December 14, 2017 15:54

Try to fix submenu issues

abf2f84

Delete minimal class, use minimal prop instead

574813a

Add a <span> wrapper around popover targets/overlays, fix some submen…

bc14014

…u styles

Set minimal=true in the docs navigator, instead of providing .pt-mini…

2da72ab

…mal className

Move <MenuItem> minimal prop below the spread

410aa12

Add popoverRef prop

7ddb55b

Fix left-aligned menus

d0fbfa6

Clarifying comment about ALIGN_LEFT

f4fd197

Merge branch 'master' into cl/graduate-labs-to-core-2

4ef17d3

Fix lingering popover2 mention in popover.md

611aadb

Merge branch 'master' into cl/graduate-labs-to-core-2

fc3b0c8

cmslewis added 2 commits December 18, 2017 14:55

No globs

7ecbb73

Cache bust: v5 => v6

63d3564

cmslewis mentioned this pull request Dec 18, 2017

Circle CI build improvements #1939

Closed

adidahiya reviewed Dec 18, 2017

View reviewed changes

Undo singlRun flag change

1bb19e1

cmslewis added 2 commits December 18, 2017 15:43

Revert changes to Circle config file

d60d61a

Fix DateRangeInput test

03d0e89

Fix table tests

08312ff

cmslewis commented Dec 19, 2017

View reviewed changes

Clewis self-review cleanups

ab65a37

giladgray reviewed Dec 19, 2017

View reviewed changes

Fix build (was due to errant find+replace), fix comment

91977ec

adidahiya previously requested changes Dec 19, 2017

View reviewed changes

Fix inline popover targets (CSS)

2e26173

cmslewis changed the base branch from master to cl/graduate-labs-to-core December 19, 2017 19:26

cmslewis merged commit 351649c into cl/graduate-labs-to-core Dec 19, 2017

cmslewis deleted the cl/graduate-labs-to-core-2 branch December 19, 2017 19:52

Graduate Labs Components - Part 2: Replace Popover/Tooltip with Popover2/Tooltip2 #1891

Graduate Labs Components - Part 2: Replace Popover/Tooltip with Popover2/Tooltip2 #1891

Conversation

cmslewis commented Dec 12, 2017 • edited Loading

Next steps (each in a separate PR, probably):

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

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

blueprint-bot commented Dec 18, 2017

Add 'packages/*/node_modules/' to save_cache dirs

blueprint-bot commented Dec 18, 2017

Merge branch 'master' into cl/graduate-labs-to-core-2

blueprint-bot commented Dec 18, 2017

Cache bust: v5 => v6

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Dec 18, 2017

Undo singlRun flag change

blueprint-bot commented Dec 18, 2017

Revert changes to Circle config file

blueprint-bot commented Dec 19, 2017

Fix table tests

cmslewis commented Dec 19, 2017 • edited Loading

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

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 Dec 19, 2017

Fix build (was due to errant find+replace), fix comment

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

blueprint-bot commented Dec 19, 2017

Fix inline popover targets (CSS)

cmslewis commented Dec 19, 2017

cmslewis commented Dec 19, 2017

cmslewis commented Dec 12, 2017 •

edited

Loading

cmslewis commented Dec 19, 2017 •

edited

Loading