fix(modal): support close animation #1417
Conversation
|
@pkozlowski-opensource , I understand your concern about the use of class names in the code. I've updated my PR. I guess in the future, there might be more cases of using classes in the directive code when we switch to using ngAnimate but we can follow the |
|
@chrisirhc had a look at the code and it LGTM now. Then I've run the demo page and saw some strange effects (it looked like backdrop isn't animated or something). Need to take a closer look before merging. |
|
Hm.. Just checked back on it and I don't see anything weird. However, I did notice that the modal animation on Twitter Bootstrap actually waits for the modal to be hidden first before animating the backdrop. Right now, what I'm doing is animating both of them at the same time. I'll probably re-look into that later. |
Tests now have to wait for animation before checking for open modals. Tests should actually check for the 'in' class. Setting up both the transition end handler and a timeout ensures that the modal is always closed even if there was an issue with the transition. This was the implementation used by Twitter Bootstrap modal JS code. Note that unbinding the transition end event within the event handler isn't necessary, and, worse is currently buggy in jqLite (see angular/angular.js#5109 ). Note that the scope is already destroyed when the dom is removed so the $destroy call isn't needed. Closes angular-ui#1341
|
@pkozlowski-opensource updated this PR for Bootstrap 3 and modal leak fixes. |
|
@chrisirhc AWESOME! Landed as 1933488 |
Closes #1341
The second commit, to fix tests isn't necessary. It's here just for thoughts as the
inclass determines whether the modal or backdrop is actually visible.Notes:
I'm not using the
$transitionmethod itself to create any transitions. I'm actually using the transition module just to determine if the browser supports transitions. Checking for transition support isn't actually necessary as modal still closes within 500ms due to the timeout fallback mechanism.I understand that it's a little hacky that I did a
removeClass('in')within the remove logic instead of attempting to change thescope.animate. However, changing the scope.animate for the modal isn't trivial as the modelWindow directive is using an isolate scope. Since the scope has been destroyed, there won't be a conflict between states, the ng-class on modelBackdrop and modelWindow directives no longer have any effect.I realized that isn't any need for multiple backdrop element instances and that the element can be left in the DOM after its first creation, and hidden instead of being removed. This can be a future fix.