Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

refactor(modal): use ngAnimate for animation #2724

Merged
merged 1 commit into from Mar 23, 2015

Conversation

chrisirhc
Copy link
Contributor

Supercedes #1772

This makes use of the new $animate:close event to determine that the animation is done instead of using $transition. There are minimal changes to the current code and there's one fix and the removal of some timers.

@OnkelTem
Copy link

This would be very cool thing! Can't wait!


domEl.bind(transitionEndEventName, function () {
$timeout.cancel(timeout);
domEl.on('$animate:close', function closeFn() {
afterAnimating();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to do this via scope.$evalAsync .

@chrisirhc chrisirhc force-pushed the feature/animate-modal-2 branch 2 times, most recently from 957ceab to cd30758 Compare October 20, 2014 10:07
@chrisirhc
Copy link
Contributor Author

This code is ready to merge. It's just blocked on #2699 as the carousel is broken after adding ngAnimate.

@ilanbiala
Copy link

Can we get this merged?

@chrisirhc chrisirhc modified the milestones: 0.12, 0.13 Nov 16, 2014
@Rush
Copy link

Rush commented Feb 1, 2015

I'd like to see it merged as well. :)

@karianna
Copy link
Contributor

karianna commented Feb 1, 2015

@chrisirhc - it can no longer merge automatically.

@chrisirhc
Copy link
Contributor Author

Should fix #3182 as well. I plan to rebase this once #2699 is merged.

@chrisirhc
Copy link
Contributor Author

Rebased. Now if anyone can take a look, I'll merge this.

$timeout.cancel(timeout);
afterAnimating();
scope.$apply();
// TODO: use .one when upgrading to >= 1.2.21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could version sniff (angular.version.major, angular.version.minor, and angular.version.dot) here to handle the differences properly - thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, since we're requiring 1.3 after this release, .one is already usable. I'll remove this and use .one.

@wesleycho
Copy link
Contributor

Other than that one minor comment (may be a non-issue), this PR LGTM.

chrisirhc added a commit to chrisirhc/angular-ui-bootstrap that referenced this pull request Mar 23, 2015
- Fix scope.$apply call not working previously because scope was already
  destroyed. Use $rootScope instead.
- Move $destroy call nearer to the dom removal.
- Remove fallback timer (emulateTime param)

Fixes angular-ui#2585
Fixes angular-ui#2674
Fixes angular-ui#2536
Fixes angular-ui#2790
Fixes angular-ui#3182
Closes angular-ui#2724
- Fix scope.$apply call not working previously because scope was already
  destroyed. Use $rootScope instead.
- Move $destroy call nearer to the dom removal.
- Remove fallback timer (emulateTime param)

Fixes angular-ui#2585
Fixes angular-ui#2674
Fixes angular-ui#2536
Fixes angular-ui#2790
Fixes angular-ui#3182
Closes angular-ui#2724
@chrisirhc chrisirhc merged commit 0d8820b into angular-ui:master Mar 23, 2015
@chrisirhc chrisirhc deleted the feature/animate-modal-2 branch March 23, 2015 02:19
@wesleycho wesleycho mentioned this pull request Mar 23, 2015
ayakovlevgh pushed a commit to ayakovlevgh/bootstrap that referenced this pull request Mar 24, 2015
- Fix scope.$apply call not working previously because scope was already
  destroyed. Use $rootScope instead.
- Move $destroy call nearer to the dom removal.
- Remove fallback timer (emulateTime param)

Fixes angular-ui#2585
Fixes angular-ui#2674
Fixes angular-ui#2536
Fixes angular-ui#2790
Fixes angular-ui#3182
Closes angular-ui#2724
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants