Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($animate): proper support for multiple elements #5570

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chrisirhc
Copy link
Contributor

Run performAnimation for each node instead of doing it for the first
element node.

Update: Made this less hacky. I made this PR keeping in mind to make as little changes in animate.js and as quickly as possible to get across my point. The code can be more elegant (e.g. introduce another function, etc.).

By running the animation on a per-element basis, there will be no issue with comment nodes etc. and the animation methods are now more intuitive and less surprising (more explanation in #5561 for what I'm referring to).

Fixes #4954 , relates to #5561 and #4786 .

Note that this change makes the animations method behaviors asymmetric. When calling $animate.leave(elements), the leave animation to fire for each element in elements. However, as the current implementation of animations is class-based, I don't see any value in firing the leave animation for non-element nodes as they cannot have classes and hence, have no meaning to ngAnimate.

@chrisirhc
Copy link
Contributor Author

@matsko , I've updated the PR. I introduced an element argument for the domOperation method as those have to be called per-element in order for JS animations to work as expected. To make things clearer, I used contents in the performAnimation method signature rather than element as it takes in jQuery/jqLite objects that may contain zero to many elements (for example, only comment nodes or only text nodes). Perhaps the method signatures for the animation methods should be updated to contents as well. I'll await your opinion on this.

The extractElementNode is quite redundant now. The default '' animateProvider no longer needs to use extractElementNode as the element should always be just one element node.
However, I'm not sure if users will use the replaceWith() method in their animation and affect this assumption.

@matsko
Copy link
Contributor

matsko commented Jan 6, 2014

@chrisirhc great work. I will be getting to this in a few days. Sorry for the wait.

@chrisirhc
Copy link
Contributor Author

@matsko , no problem. Just noticed that the tests didn't run due to a jshint warning and updated the PR again.

@chrisirhc
Copy link
Contributor Author

Fixed test where IE8 was throwing Object doesn't support this property or method when calling .enabled on contents that contain non-element nodes.

@chrisirhc
Copy link
Contributor Author

@matsko Tests are passing now, after I rebased it and used triggerReflow.

@matsko
Copy link
Contributor

matsko commented Jan 22, 2014

Thank you @chrisirhc for updating this close to master. I will be reviewing this today/tomorrow.

@matsko
Copy link
Contributor

matsko commented Aug 11, 2014

@chrisirhc there may be a slight API change for ngAnimate to use more properties. That means having to use multiple elements may also be put into consideration. I will reply to this thread by the end of the week to let you know what the status is on that.

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.

ngAnimate insertBefore error
3 participants