Fix <HotkeyDialog /> exit animation #221

cmslewis · 2016-11-22T01:18:48Z

PR checklist

New bugfix

What changes did you make?

Animate the <HotkeyDialog>'s exit transition, per the default <Dialog /> behavior, rather than hiding the dialog immediately.

Is there anything you'd like reviewers to focus on?

The DOM is getting cleared and repopulated appropriately with each call to show()/hide():

But worth investigating more: are we leaking memory with each successive invocation to ReactDOM.render? Do we need to explicitly delete the old instance of the rendered element?

… locally)

…ssing

blueprint-bot · 2016-11-22T01:57:00Z

Try 1s wait

Preview: docs Coverage: core | datetime

blueprint-bot · 2016-11-29T01:50:20Z

Add explanatory comment to the dialog unit test

Preview: docs | table Coverage: core | datetime

blueprint-bot · 2016-11-29T21:51:21Z

Merge branch 'master' into cl/hotkey-dialog

Preview: docs Coverage: core | datetime

cmslewis · 2016-11-30T23:15:12Z

@themadcreator can you take a look at this? want to make sure everything looks good by your eyes, since you were the author.

themadcreator

Some minor changes requested

themadcreator · 2016-12-01T00:57:15Z

packages/core/src/components/hotkeys/hotkeysDialog.tsx

-    public showing = false;
+
+    private container: HTMLElement;
+    private component: React.Component<any, React.ComponentState>;


this is saved off in render but never used AFAICT, so might as well remove it

themadcreator · 2016-12-01T00:59:23Z

packages/core/src/components/hotkeys/hotkeysDialog.tsx

+        if (this.container == null) {
+            this.container = this.getContainer();
+        }
+        const element = this.renderComponent();


inline this in the next line

blueprint-bot · 2016-12-01T01:18:54Z

Fix nits per PR comments

Preview: docs
Coverage: core | datetime

cmslewis · 2016-12-01T01:36:56Z

@themadcreator changes addressed. LMK if this looks good now.

giladgray

this looks great, just waiting for bill. and one thing about tests that would be great to fix

giladgray · 2016-12-01T01:47:45Z

packages/core/test/hotkeys/hotkeysTests.tsx

@@ -114,19 +114,32 @@ describe("Hotkeys", () => {
        });

        it("triggers hotkey dialog with \"?\"", (done) => {
+            // this test takes awhile. the hotkey dialog is supposed to take 100ms


ugh we really want to avoid unavoidably long tests like this, it just slows the whole thing down.

a better approach would be to assert that props are what you'd expect via the enzyme wrapper, rather than waiting for the DOM to catch up.

not only does it slow the test suite down -- it also introduces flakiness because these timeouts are dependent on things like CPU load. I would probably prefer to skip this test until we have test-friendly minimal dialogs

@cmslewis please change to it.skip(...) to ignore this test and file an issue to track refactoring it.

blueprint-bot · 2016-12-02T20:25:08Z

Edit test to not check for hiding behavior (takes too long)

Preview: docs | table
Coverage: core | datetime

* Fix HotkeyDialog's closing transition * Edit test to not check for hiding behavior (takes too long)

cmslewis added 2 commits November 21, 2016 17:11

Fix HotkeyDialog's closing transition

178b577

Reorder props

05644ae

cmslewis added Domain: documentation Package: core PR: waiting for reviewer Type: bug labels Nov 22, 2016

cmslewis added 5 commits November 21, 2016 17:23

Attempt to fix tests

97a09ab

Try again to fix tests (need to figure out how to unbreak 'gulp test'…

485c3c6

… locally)

Add some console.logs temporarily. Ugh.

9fd7e02

Try 1s wait

8b6d9ec

Remove console log and reduce extra-wait factor now that tests are pa…

43fa838

…ssing

Stop testing for hiding

9fe169e

cmslewis force-pushed the cl/hotkey-dialog branch from d3f92d2 to 9fe169e Compare November 22, 2016 02:02

adidahiya removed the PR: waiting for reviewer label Nov 22, 2016

adidahiya assigned cmslewis Nov 22, 2016

cmslewis added 5 commits November 21, 2016 23:43

Remove unused import

4eea043

Merge branch 'master' into cl/hotkey-dialog

53b594a

Fix hotkeys tests

4b6bf2d

Add explanatory comment to the dialog unit test

7d9465a

Merge branch 'master' into cl/hotkey-dialog

092f1a0

Merge branch 'master' into cl/hotkey-dialog

27979b9

cmslewis removed their assignment Nov 29, 2016

llorca assigned themadcreator Nov 30, 2016

themadcreator suggested changes Dec 1, 2016

View reviewed changes

cmslewis added 2 commits November 30, 2016 17:05

Merge branch 'master' into cl/hotkey-dialog

cc39b0c

Fix nits per PR comments

75770d9

giladgray reviewed Dec 1, 2016

View reviewed changes

themadcreator approved these changes Dec 1, 2016

View reviewed changes

cmslewis added 2 commits December 1, 2016 18:52

Skip the unit test for now

e64fe42

Add explanatory comment in unit test

1abb51b

adidahiya modified the milestone: 1.2.0 Dec 2, 2016

Edit test to not check for hiding behavior (takes too long)

4d66b7b

giladgray merged commit d381dbb into master Dec 2, 2016

giladgray deleted the cl/hotkey-dialog branch December 2, 2016 20:27

adidahiya mentioned this pull request Dec 2, 2016

Test-friendly dialog animations #301

Closed

llorca mentioned this pull request Dec 2, 2016

Prepare release v1.2.0 #302

Merged

cmslewis restored the cl/hotkey-dialog branch December 6, 2016 00:10

adidahiya deleted the cl/hotkey-dialog branch December 6, 2016 18:53

greglo pushed a commit to greglo/blueprint that referenced this pull request Dec 12, 2016

Fix <HotkeyDialog /> exit animation (palantir#221)

64eebfb

* Fix HotkeyDialog's closing transition * Edit test to not check for hiding behavior (takes too long)

snyk-bot mentioned this pull request Apr 15, 2020

[Snyk] Security upgrade karma-mocha from 1.3.0 to 2.0.0 maxiplux/blueprint#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix <HotkeyDialog /> exit animation #221

Fix <HotkeyDialog /> exit animation #221

cmslewis commented Nov 22, 2016

blueprint-bot commented Nov 22, 2016

blueprint-bot commented Nov 29, 2016

blueprint-bot commented Nov 29, 2016

cmslewis commented Nov 30, 2016

themadcreator left a comment

themadcreator Dec 1, 2016

cmslewis Dec 1, 2016

themadcreator Dec 1, 2016

cmslewis Dec 1, 2016

blueprint-bot commented Dec 1, 2016

cmslewis commented Dec 1, 2016

giladgray left a comment

giladgray Dec 1, 2016

adidahiya Dec 1, 2016

giladgray Dec 1, 2016

cmslewis Dec 2, 2016 •

edited

Loading

blueprint-bot commented Dec 2, 2016

Fix <HotkeyDialog /> exit animation #221

Fix <HotkeyDialog /> exit animation #221

Conversation

cmslewis commented Nov 22, 2016

PR checklist

What changes did you make?

Is there anything you'd like reviewers to focus on?

blueprint-bot commented Nov 22, 2016

Try 1s wait

blueprint-bot commented Nov 29, 2016

Add explanatory comment to the dialog unit test

blueprint-bot commented Nov 29, 2016

Merge branch 'master' into cl/hotkey-dialog

cmslewis commented Nov 30, 2016

themadcreator left a comment

Choose a reason for hiding this comment

themadcreator Dec 1, 2016

Choose a reason for hiding this comment

cmslewis Dec 1, 2016

Choose a reason for hiding this comment

themadcreator Dec 1, 2016

Choose a reason for hiding this comment

cmslewis Dec 1, 2016

Choose a reason for hiding this comment

blueprint-bot commented Dec 1, 2016

Fix nits per PR comments

cmslewis commented Dec 1, 2016

giladgray left a comment

Choose a reason for hiding this comment

giladgray Dec 1, 2016

Choose a reason for hiding this comment

adidahiya Dec 1, 2016

Choose a reason for hiding this comment

giladgray Dec 1, 2016

Choose a reason for hiding this comment

cmslewis Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

blueprint-bot commented Dec 2, 2016

Edit test to not check for hiding behavior (takes too long)

cmslewis Dec 2, 2016 •

edited

Loading