fix(dialog): Closing Animations not running #433 #504

tranrate · 2017-04-11T14:03:28Z

Refactor scss to include Animating class.
Modify foundation to use Animating class and transitionEnd event for opening and closing Dialog. Alter open close and destroy methods appropriately.
Move style="visibility:hidden" attribute into scss so that it is not required in html.

Closes #433

BREAKING CHANGE:

Dialogs do not require a style="visibility:hidden" attribute in html.
registerTransitionEndHandler, deregisterTransitionEndHandler, and isDialog methods must be implemented by the adapter

codecov-io · 2017-04-11T14:14:27Z

Codecov Report

Merging #504 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   99.91%   99.92%   +<.01%     
==========================================
  Files          59       59              
  Lines        2482     2502      +20     
  Branches      288      291       +3     
==========================================
+ Hits         2480     2500      +20     
  Misses          2        2

Impacted Files	Coverage Δ
packages/mdc-dialog/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-dialog/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-dialog/index.js	`100% <100%> (ø)`	⬆️

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 a5c6542...35c92bb. Read the comment docs.

amsheehan

This all looks really good. A couple of quick notes, but that's it from me. Thanks for the great work!

I think the only thing remaining is to update the demo to remove the style="visibility: hidden" attribute from the dialog elements. 😛

After that, LGTM 🤙

amsheehan · 2017-05-10T20:27:15Z

packages/mdc-dialog/foundation.js

@@ -118,6 +128,18 @@ export default class MDCDialogFoundation extends MDCFoundation {
    }
  }

+  handleTransitionEnd_(ev) {


Ultra-nit: We usually call this evt

amsheehan · 2017-05-10T20:32:02Z

packages/mdc-dialog/mdc-dialog.scss

+  width: 100%;
+  height: 100%;
+  visibility: hidden;
+  z-index: 2;


z-index is set on line 54 as well. I believe this one is extraneous.

amsheehan · 2017-05-10T20:36:39Z

packages/mdc-dialog/mdc-dialog.scss

@@ -131,7 +133,6 @@ $mdc-dialog-dark-theme-bg-color: #303030 !default;
    padding: 8px;
  }

-  // TODO: Replace with breakpoint variable


We would like to keep this comment in for now. // style comments are stripped out in the compiled CSS, so this only really exists for us in dev. These will all be removed once #23 lands.

tranrate · 2017-05-16T17:27:28Z

Hi @amsheehan

Demo updated - conflicts with Revamp Demo/Catalog Revamp Demo/Catalog #594 resolved.
Foundation.js line 131 - ev changed to evt.
Mdc-dialog.scss line 134 - Comment added back in.
Mdc-dialog.scss line 40 - z-index left as it is, the second one is for the opaque backdrop which has to be behind the dialog box but in front of the main screen. Used a relative index for the backdrop so that only the main one needs changing if anyone needs to adjust the z-index in the future.

Martin

traviskaufman · 2017-05-18T15:54:02Z

@amsheehan could we get another review here?

Also note that in the commit message body we need to change BREAKING CHANGES: to BREAKING CHANGE:

amsheehan

Once the commit message is changed to BREAKING CHANGE: this is good to merge.

tranrate · 2017-05-20T00:41:44Z

@amsheehan

Commit message changed.

Martin

googlebot · 2017-05-22T15:52:34Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

* Official breaking changes from material-web-components material-components/material-components-web#504 material-components/material-components-web@5b5ea95 * Fix transition events in dialog

tranrate added 13 commits April 11, 2017 02:41

Update README.md

9be279d

Update README.md

9faa040

Update constants.js

1267e72

Update foundation.js

5b5ea95

Update index.js

6968601

Update mdc-dialog.scss

cf41d2a

Update foundation.test.js

611e77b

Update mdc-dialog.test.js

9a63d19

Update mdc-dialog.scss

1f785b1

Update foundation.js

d635194

Update foundation.test.js

f3c4178

Update mdc-dialog.test.js

f6d7e0b

Update foundation.test.js

ddc5f2a

traviskaufman self-assigned this Apr 21, 2017

amsheehan reviewed May 10, 2017

View reviewed changes

amsheehan self-assigned this May 10, 2017

tranrate added 8 commits May 15, 2017 20:08

Update foundation.js

9223dab

Update mdc-dialog.scss

c5002b1

Update dialog.html

db77ea7

Update dialog.html

8a72332

Create demos.css

076c882

Update demos.css

fcb24ff

Update dialog.html

c13f92e

Merge branch 'master' into patch-5

903f8d6

Merge branch 'master' into patch-5

311d479

Merge branch 'master' into patch-5

1e2b98b

amsheehan suggested changes May 19, 2017

View reviewed changes

Merge branch 'master' into patch-5

a914ebe

tranrate changed the title ~~Fix (Dialog) Closing Animations not running #433~~ Fix(Dialog): Closing Animations not running #433 May 20, 2017

tranrate changed the title ~~Fix(Dialog): Closing Animations not running #433~~ Fix(dialog): Closing Animations not running #433 May 20, 2017

tranrate changed the title ~~Fix(dialog): Closing Animations not running #433~~ fix(dialog): Closing Animations not running #433 May 20, 2017

Merge branch 'master' into patch-5

35c92bb

amsheehan merged commit 2b03c6b into material-components:master May 22, 2017

This was referenced Jul 2, 2017

MDCDialogFoundation not removing mdc-dialog--animating when closing #893

Closed

Official breaking changes from material-web-components posva/vue-mdc#218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(dialog): Closing Animations not running #433 #504

fix(dialog): Closing Animations not running #433 #504

Uh oh!

tranrate commented Apr 11, 2017 •

edited

Loading

Uh oh!

codecov-io commented Apr 11, 2017 •

edited

Loading

Uh oh!

amsheehan left a comment •

edited

Loading

Uh oh!

amsheehan May 10, 2017

Uh oh!

amsheehan May 10, 2017

Uh oh!

amsheehan May 10, 2017

Uh oh!

tranrate commented May 16, 2017

Uh oh!

traviskaufman commented May 18, 2017

Uh oh!

amsheehan left a comment

Uh oh!

tranrate commented May 20, 2017

Uh oh!

googlebot commented May 22, 2017

Uh oh!

Uh oh!

fix(dialog): Closing Animations not running #433 #504

fix(dialog): Closing Animations not running #433 #504

Uh oh!

Conversation

tranrate commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amsheehan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amsheehan May 10, 2017

Choose a reason for hiding this comment

Uh oh!

amsheehan May 10, 2017

Choose a reason for hiding this comment

Uh oh!

amsheehan May 10, 2017

Choose a reason for hiding this comment

Uh oh!

tranrate commented May 16, 2017

Uh oh!

traviskaufman commented May 18, 2017

Uh oh!

amsheehan left a comment

Choose a reason for hiding this comment

Uh oh!

tranrate commented May 20, 2017

Uh oh!

googlebot commented May 22, 2017

Uh oh!

Uh oh!

tranrate commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading

amsheehan left a comment •

edited

Loading