Animate the SymbolModal's entrance/exit by wldcordeiro · Pull Request #3415 · firefox-devtools/debugger

wldcordeiro · 2017-07-22T18:27:54Z

Associated Issue: #3234

Summary of Changes

Break the Modal itself into it's own component
Animate that component
Use it in SymbolModal

Test Plan

cmd+shift+O to open
See the modal slide in
escape to close
See the modal slide out

Screenshots/Videos

codecov · 2017-07-22T18:52:03Z

Codecov Report

Merging #3415 into next will decrease coverage by 0.18%.
The diff coverage is 16%.

@@           Coverage Diff            @@
##           next    #3415      +/-   ##
========================================
- Coverage    50%   49.81%   -0.19%     
========================================
  Files       107      108       +1     
  Lines      4460     4483      +23     
  Branches    920      919       -1     
========================================
+ Hits       2230     2233       +3     
- Misses     2230     2250      +20

Impacted Files	Coverage Δ
src/components/SymbolModal.js	`31.53% <100%> (+0.24%)`	⬆️
src/components/shared/Modal.js	`12.5% <12.5%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4691d7a...8356b51. Read the comment docs.

codecov · 2017-07-22T18:52:03Z

Codecov Report

❗ No coverage uploaded for pull request base (next@9b3b2c6). Click here to learn what that means.
The diff coverage is 20.83%.

@@          Coverage Diff           @@
##             next   #3415   +/-   ##
======================================
  Coverage        ?   51.5%           
======================================
  Files           ?     110           
  Lines           ?    4565           
  Branches        ?     943           
======================================
  Hits            ?    2351           
  Misses          ?    2214           
  Partials        ?       0

Impacted Files	Coverage Δ
src/components/shared/Modal.js	`10.52% <10.52%> (ø)`
src/components/SymbolModal.js	`31.53% <60%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b3b2c6...14cbd24. Read the comment docs.

wldcordeiro · 2017-07-22T20:35:51Z

I'm not sure how to resolve these flow issues as the Modal doesn't really have any typings different from other components. Then as for yarn check, I can't seem to resolve that either. Seems like some dependency of storybook isn't happy with the react version we have?

codehag · 2017-07-24T10:23:40Z

I will take a look again later, but from a quick reading it looks like you are expecting a single node as a child, and passing two.. i will take another look later in the evening.

jasonLaster

Very cool. Just concerned about the wrapper overlaying the window.

jasonLaster · 2017-07-24T12:52:30Z

+    return Slide({
+      in: enabled,
+      children: status => {
+        return dom.div(


It might help if the wrapper and modal were siblings. This way the modal could animate onto the <body>.

Not sure I follow. Could you elaborate?

jasonLaster · 2017-07-24T12:53:10Z

+.modal-wrapper.entered,
+.modal-wrapper.exiting {
+  pointer-events: auto;
+}


we should avoid having a div overlaying the window at all times.

another strategy might be to use z-index: -1.

Good idea on moving the z-index.

wldcordeiro · 2017-07-24T18:52:39Z

I will take a look again later, but from a quick reading it looks like you are expecting a single node as a child, and passing two.. i will take another look later in the evening.

@codehag I was looking into how to make a function have variadic arity in Flow but I'm not finding a good resource. 🙁 I'll keep digging.

wldcordeiro · 2017-07-24T19:15:38Z

+  z-index: -1;
+  top: 0;
+  left: 0;
+  transition: z-index 0.4s ease-in-out;


I added this slightly longer transition to the z-index change to make sure we have enough time for the animation to play.

codehag · 2017-07-25T15:56:26Z

fixed up the flow types, was a tough case!

jasonLaster · 2017-07-25T16:39:24Z

➜  debugger.html yarn check
yarn check v0.24.5
warning "chokidar#fsevents#node-pre-gyp@^0.6.36" could be deduped from "0.6.36" to "node-pre-gyp@0.6.36"
error "react-modal#react-dom-factories#react@^15.5.4" doesn't satisfy found match of "react@15.3.2"
error Found 1 errors.
info Visit https://yarnpkg.com/en/docs/cli/check for documentation about this command.

we might need a lower version of transition-group

wldcordeiro · 2017-07-25T16:56:50Z

@codehag thanks for that! @jasonLaster the transition-group package.json seems to indicate we would have a good version. https://github.com/reactjs/react-transition-group/blob/master/package.json#L44 We could try rolling back to a different version but the 1.x has a slightly different API so I'd need to modify a bit further.

jasonLaster · 2017-07-25T17:03:08Z

Let's try a 1.x if that makes yarn happy. Basically downgrade do yarn check, then maybe move forward

…

On Tue, Jul 25, 2017 at 1:01 PM Wellington Cordeiro < ***@***.***> wrote: @codehag <https://github.com/codehag> thanks for that! @jasonLaster <https://github.com/jasonlaster> the transition-group package.json seems to indicate we would have a good version. https://github.com/reactjs/react-transition-group/blob/master/package.json#L44 We could try rolling back to a different version but the 1.x has a slightly different API so I'd need to modify a bit further. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPiYnfrIdz_YciswSJI-lpBwrPl4tZBks5sRh9zgaJpZM4OgPqz> .

wldcordeiro · 2017-07-25T17:13:46Z

I tried dropping to 1.x and still saw the same error. 🙁

jasonLaster · 2017-07-25T18:04:43Z

What do we need this dependency to do? Why not just add a class that has the transition?

…

On Tue, Jul 25, 2017 at 1:19 PM Wellington Cordeiro < ***@***.***> wrote: I tried dropping to 1.x and still saw the same error. 🙁 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPiYja4drxFi_w0lZHKig4CiuBR4T_7ks5sRiNIgaJpZM4OgPqz> .

codehag · 2017-07-26T10:05:17Z

I have a fix, just waiting for tests to pass

codehag · 2017-07-26T10:58:12Z

This has been rebased to remove the offending yarn.lock inclusion -- if you modify it please pull this branch first and replace your local branch

wldcordeiro · 2017-07-26T15:50:31Z

@codehag I pulled changes, rebased onto next and had to revert the yarn.lock entirely because even removing my dep and regenerating caused the error in yarn check

codehag · 2017-07-26T16:06:56Z

@wldcordeiro try this:

go to master
delete your local branch
checkout your local branch again

this will give you a clean version

I suspect that you still have the old yarn.lock file and that is what is causing the troubles. It is building on CI now, and also locally for me

wldcordeiro · 2017-07-26T22:15:10Z

@codehag @jasonLaster with the changes you've made I think this is ready. I have nothing else I need to modify. There are some changes in the PR though that seem to have come from elsewhere. 🙁

wldcordeiro · 2017-07-28T07:16:05Z

Okay I think now the rebase madness is done.

commit 8ac8235004fb08a1bb0ab43c2f2e704f88ef52b3 (HEAD -> animate-modal, origin/animate-modal)
Author: Wellington Cordeiro <wellington@wellingtoncordeiro.com>
Date:   Tue Jul 25 16:14:25 2017 -0600

    Adjust animation to use Photon recommendations.

commit d59b66d7cdd6d34b7a118d984d466b5772d40e55
Author: Jason Laster <jason.laster.11@gmail.com>
Date:   Tue Jul 25 16:52:08 2017 -0400

    drop duplicate close handler

commit f0a64aba8325f13f8f69dc3b45886f7c031c6633
Author: Wellington Cordeiro <wellington@wellingtoncordeiro.com>
Date:   Tue Jul 25 12:45:55 2017 -0600

    Remove react-transition-group and just do the animation via the enabled prop and CSS

commit f333e84d4f27fdd8e72689ce8ce2421850c3bf43
Author: Jason Laster <jason.laster.11@gmail.com>
Date:   Tue Jul 25 12:33:25 2017 -0400

    fixup yarn

commit d6000c260a7b2dc13041e3c07460f6c44e6ecb50
Author: codehag <yulia.startsev@gmail.com>
Date:   Tue Jul 25 17:54:49 2017 +0200

    make flow happy

commit dcebe99b0ab1585c83a0abaf81f380284201d31d
Author: Wellington Cordeiro <wellington@wellingtoncordeiro.com>
Date:   Mon Jul 24 13:11:30 2017 -0600

    Use z-index rather than pointer-events to move the .modal-wrapper

commit c2fc6b3b88719dcdfc45ce93b3ff05bd1414ca6c
Author: Wellington Cordeiro <wellington@wellingtoncordeiro.com>
Date:   Fri Jul 28 01:06:39 2017 -0600

    Animate symbol modal using react-transition-group

…ed prop and CSS

wldcordeiro · 2017-07-28T15:57:11Z

The commit log here leaves evidence that I'm a time traveler. 😆

* Animate symbol modal using react-transition-group * Use z-index rather than pointer-events to move the .modal-wrapper * fixup yarn * Remove react-transition-group and just do the animation via the enabled prop and CSS * Adjust animation to use Photon recommendations. * drop duplicate close handler * make flow happy

wldcordeiro force-pushed the animate-modal branch from d07bbca to 8356b51 Compare July 22, 2017 18:52

wldcordeiro force-pushed the animate-modal branch 2 times, most recently from 7cf448d to a7d8f3b Compare July 22, 2017 20:33

wldcordeiro force-pushed the animate-modal branch from a7d8f3b to cbd4271 Compare July 23, 2017 06:22

jasonLaster reviewed Jul 24, 2017

View reviewed changes

wldcordeiro force-pushed the animate-modal branch from cbd4271 to db227ed Compare July 24, 2017 18:53

wldcordeiro commented Jul 24, 2017

View reviewed changes

wldcordeiro force-pushed the animate-modal branch from 0b5e3da to 8175039 Compare July 24, 2017 19:17

jasonLaster force-pushed the animate-modal branch from b293364 to 7d27e2f Compare July 25, 2017 16:37

wldcordeiro force-pushed the animate-modal branch 3 times, most recently from cc1140d to 98a6b33 Compare July 26, 2017 01:05

codehag force-pushed the animate-modal branch from 98a6b33 to 4ddce7b Compare July 26, 2017 10:58

wldcordeiro force-pushed the animate-modal branch from 4ddce7b to 535518b Compare July 28, 2017 07:09

wldcordeiro force-pushed the animate-modal branch from 535518b to 8ac8235 Compare July 28, 2017 07:39

codehag force-pushed the animate-modal branch from 8ac8235 to c50e164 Compare July 28, 2017 14:25

wldcordeiro and others added 7 commits July 28, 2017 16:27

Animate symbol modal using react-transition-group

3bafca8

Use z-index rather than pointer-events to move the .modal-wrapper

922ed26

make flow happy

7c699e6

fixup yarn

95a02c1

Remove react-transition-group and just do the animation via the enabl…

7c14d2d

…ed prop and CSS

drop duplicate close handler

64b141b

Adjust animation to use Photon recommendations.

14cbd24

codehag force-pushed the animate-modal branch from c50e164 to 14cbd24 Compare July 28, 2017 14:30

codehag merged commit 829954d into firefox-devtools:next Jul 28, 2017

wldcordeiro deleted the animate-modal branch July 28, 2017 15:44

Conversation

wldcordeiro commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Test Plan

Screenshots/Videos

Uh oh!

codecov Bot commented Jul 22, 2017

Codecov Report

Uh oh!

codecov Bot commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wldcordeiro commented Jul 22, 2017

Uh oh!

codehag commented Jul 24, 2017

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

jasonLaster Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

wldcordeiro Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasonLaster Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

wldcordeiro Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

wldcordeiro commented Jul 24, 2017

Uh oh!

wldcordeiro Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

codehag commented Jul 25, 2017

Uh oh!

jasonLaster commented Jul 25, 2017

Uh oh!

wldcordeiro commented Jul 25, 2017

Uh oh!

jasonLaster commented Jul 25, 2017 via email

Uh oh!

wldcordeiro commented Jul 25, 2017

Uh oh!

jasonLaster commented Jul 25, 2017 via email

Uh oh!

codehag commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codehag commented Jul 26, 2017

Uh oh!

wldcordeiro commented Jul 26, 2017

Uh oh!

codehag commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wldcordeiro commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wldcordeiro commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wldcordeiro commented Jul 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wldcordeiro commented Jul 22, 2017 •

edited

Loading

codecov Bot commented Jul 22, 2017 •

edited

Loading

codehag commented Jul 26, 2017 •

edited

Loading

codehag commented Jul 26, 2017 •

edited

Loading

wldcordeiro commented Jul 26, 2017 •

edited

Loading

wldcordeiro commented Jul 28, 2017 •

edited

Loading