feat($animate): add support for `customFilter` by gkalpak · Pull Request #14914 · angular/angular.js

gkalpak · 2016-07-15T11:16:29Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.

What is the new behavior (if this is a feature change)?
This commit adds a new customFilter() function on $animateProvider (similar to classNameFilter()), which can be used to filter animations (i.e. decide whether they are allowed or not), based on the return value of a custom filter function.
This allows to easily create arbitrarily complex rules for filtering animations, such as allowing specific events only, or enabling animations on specific subtrees of the DOM etc.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
Fixes #14891.

Needs docs:

Complete the API docs.
Update the Developer Guide.

petebacondarwin · 2016-07-18T08:48:16Z

+          $animateProvider.customFilter(angular.noop);
+          expect($animateProvider.customFilter()).toEqual(jasmine.any(Function));
+
+          $animateProvider.customFilter({});


perhaps we should throw instead of silently ignoring?

I kept this consistent with classNameFilter (although it is preferrable to throw obviously).
Throwing would be a BC for classNameFilter. Should I fix this one to throw and make classNameFilter throw for 1.6 (or leave it as is)?

I would change both as you suggest.

I missed that this needed to land in 1.6.0 (due to the BC) 😞
I will make only customFilter throw for 1.6.x/1.7.x and change classNameFilter to also throw for 1.7.x only.

Actually, I just realized that the current implementation allows to "clear" the filter/className by passing a non-Function/non-RegExp argument. So, if we want to throw, we need to still allow some special value (e.g. null/undefined/falsy?) for unsetting the filter/className.

petebacondarwin · 2016-07-18T08:49:22Z

One minor question - otherwise LGTM

Narretz · 2016-07-21T09:46:27Z

I like it. I would add a note though that this going to be called for every animatable action, so developers should keep the function lean.

gkalpak · 2017-01-12T12:47:44Z

I added docs for customFilter() (and a large-ish docs commit that face-lifts the Animations Guide - sorry).
I didn't make any change wrt throwing on "invalid" argument types, because of #14914 (comment).

Feel free to TAL ☺️

Narretz · 2017-01-20T18:04:20Z


+  /**
+   * @ngdoc method
+   * @name $animateProvider#customFilter


I would add two things:

How does this relate to the classFilter, .i.e. what takes precedence, or do both run

Note that this runs for every animatable action, not just for animations that are actually defined, and that the user should keep the function fast

You've added this to the guide, but I think it should be here too

How does this relate to the classFilter, .i.e. what takes precedence, or do both run

👍

Note that this runs for every animatable action, not just for animations that are actually defined, and that the user should keep the function fast

You mean in addition to the alert-box below?

If possible, I thought we should make it explicit (in the alert box) that this doesn't only affect the actual animations, but every "action" (class change, element entering). I think this difference is easily forgotten, and people might think "Oh well I don't have many animations anyway".

I see what you mean now. Good point. I will update the API docs and guide.
BTW, I think we should re-order the conditions, because now customFilter and classNameFilter are executed even if animations are globally disabled or the document is hidden etc.

Narretz · 2017-01-20T18:05:22Z

-triggered, will attempt to perform a CSS Transition, CSS Keyframe Animation or a JavaScript callback Animation (depending on if an animation is
-placed on the given directive). Animations can be placed using vanilla CSS by following the naming conventions set in place by AngularJS
-or with JavaScript code when it's defined as a factory.
+AngularJS provides animation hooks for common directives such as `ngRepeat`, `ngSwitch`, and


While you are at it, can you add links to the directives here?

Narretz · 2017-01-20T18:10:09Z

-This ensures that the whole app has been compiled fully before animations are attempted.
+Internally, `ngAnimate` waits until all template downloads that are started right after bootstrap
+have finished. Then, it waits for the currently running {@link ng.$rootScope.Scope#$digest $digest}
+and the one more after that to finish. This ensures that the whole app has been compiled fully


"and one more after that, to finish."

Narretz · 2017-01-20T18:14:48Z


 When the first argument is a native DOM or jqLite/jQuery element, the function enables / disables
 animations on this element *and all its children*: `$animate.enabled(myElement, false)`. This is the
 most flexible way to change the animation state. For example, even if you have used it to disable


Is "This is the most flexible way to change the animation state." still true with the new customFilter? I don't think so, as $animate.enabled() does not work when the classNameFilter or customFilter are active. So let's remove this sentence

Narretz · 2017-01-20T18:17:34Z



-### Enable animations for elements outside of the Angular application DOM tree: {@link ng.$animate#pin $animate.pin()}
+### Enable animations outside of the application DOM tree: {@link ng.$animate#pin $animate.pin()}


This should be ##

gkalpak · 2017-01-26T22:51:30Z

Comments addressed. PTAL

Narretz · 2017-01-30T21:31:21Z


+  /**
+   * @ngdoc method
+   * @name $animateProvider#customFilter


If possible, I thought we should make it explicit (in the alert box) that this doesn't only affect the actual animations, but every "action" (class change, element entering). I think this difference is easily forgotten, and people might think "Oh well I don't have many animations anyway".

gkalpak · 2017-01-31T11:58:56Z

Note to self: Update links after #15654.

Narretz · 2017-03-31T08:09:02Z

This looks (almost) done, wanna put it into 1.6.5?

Narretz · 2017-04-21T11:33:30Z

@gkalpak Looks like this needs a rebase. After that, I will do another review - or do you have some other changes to make?

…mprovements) - List missing animation-aware directives. - Fix/Improve wording/formatting. - Fix typos. - Limit lines to 100 chars.

…is used

This commit adds a new `customFilter()` function on `$animateProvider` (similar to `classNameFilter()`), which can be used to filter animations (i.e. decide whether they are allowed or not), based on the return value of a custom filter function. This allows to easily create arbitrarily complex rules for filtering animations, such as allowing specific events only, or enabling animations on specific subtrees of the DOM, etc. Fixes angular#14891

…lly disabled

gkalpak · 2017-04-27T12:38:48Z

I have squashed and rebased this and added two new commits:

One for further clarifying when the customFilter function is called.
One for skipping the customFilter and classNameFilter checks, if animations are globally disabled.

@Narretz PTAL

Narretz

Just two smaller docs changes needed, otherwise LGTM

Narretz · 2017-04-27T18:42:06Z

 With these generated CSS class names present on the element at the time, AngularJS automatically
-figures out whether to perform a CSS and/or JavaScript animation. If both CSS and JavaScript animation
-code is present, and match the CSS class name on the element, then AngularJS will run both animations at the same time.
+figures out whether to perform a CSS and/or JavaScript animation. If both CSS and JavaScript


That actually hasn't been true since 1.4 😱 https://code.angularjs.org/snapshot/docs/guide/migration#animation-nganimate-

So, what happens if both are detected. Who wins?

JS animations - because you can run CSS in JS: http://plnkr.co/edit/rdRAZTiXHNAFTEOHyZVo?p=preview

Although I'm surprised this isn't explicitly said in the migrate docs. But it is here: https://code.angularjs.org/snapshot/docs/api/ngAnimate

Unfortuante spreading out of content ... moving this in guide would be a good passion project for rainy afternoons. :p

Changed and added a link to that api/ngAnimate section.
There is indeed useful info there that would be better in the guide 😞

Narretz · 2017-04-27T18:43:59Z

+AngularJS also pays attention to CSS class changes on elements by triggering the **add** and
+**remove** hooks. This means that if a CSS class is added to or removed from an element then an
+animation can be executed in between, before the CSS class addition or removal is finalized.
+(Keep in mind that AngularJS will only be able to capture class changes if an **expression** or the


I think interpolated expression is clearer here

…other improvements)

…lly disabled Closes #14914

gkalpak added type: feature needs: review component: ngAnimate needs: public api severity: inconvenient labels Jul 15, 2016

gkalpak added this to the Backlog milestone Jul 15, 2016

googlebot added the cla: yes label Jul 15, 2016

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 4eb6dba to 2021884 Compare July 15, 2016 11:17

petebacondarwin reviewed Jul 18, 2016
View reviewed changes

gkalpak mentioned this pull request Jul 26, 2016

ngAnimate's class name filter can't be enabled / disabled after config time #14950

Closed

gkalpak added needs: work and removed needs: review labels Dec 16, 2016

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 2021884 to 435f690 Compare January 5, 2017 13:12

gkalpak mentioned this pull request Jan 11, 2017

ngView animation enter is not firing on page load. #15589

Closed

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 435f690 to 6b70025 Compare January 12, 2017 12:42

gkalpak added the needs: review label Jan 20, 2017

Narretz suggested changes Jan 20, 2017

View reviewed changes

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 3a78cf7 to 8c7a7b8 Compare January 26, 2017 23:14

Narretz suggested changes Jan 30, 2017

View reviewed changes

gkalpak modified the milestones: 1.6.5, Backlog Mar 31, 2017

gkalpak added 2 commits April 27, 2017 13:47

docs(guide/animations): list missing animated directives (and other i…

0596fec

…mprovements) - List missing animation-aware directives. - Fix/Improve wording/formatting. - Fix typos. - Limit lines to 100 chars.

perf($animate): do not retrieve className unless classNameFilter …

035997a

…is used

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 8c7a7b8 to 7e6660c Compare April 27, 2017 11:50

gkalpak added 2 commits April 27, 2017 15:09

fixup! feat($animate): add support for customFilter

c3397d0

perf(animate): avoid unnecessary computations if animations are globa…

237874b

…lly disabled

gkalpak force-pushed the feat-animate-support-selectorFilter branch from 7e6660c to 237874b Compare April 27, 2017 12:36

Narretz suggested changes Apr 27, 2017

View reviewed changes

fixup! docs(guide/animations): list missing animated directives (and …

33822dd

…other improvements)

Narretz approved these changes Apr 28, 2017

View reviewed changes

gkalpak closed this in 8f563e2 Apr 28, 2017

gkalpak deleted the feat-animate-support-selectorFilter branch April 28, 2017 09:54

gkalpak added a commit that referenced this pull request May 2, 2017

perf(animate): avoid unnecessary computations if animations are globa…

ce5ffbf

…lly disabled Closes #14914



		### Enable animations for elements outside of the Angular application DOM tree: {@link ng.$animate#pin $animate.pin()}
		### Enable animations outside of the application DOM tree: {@link ng.$animate#pin $animate.pin()}

Conversation

gkalpak commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak Jan 5, 2017 • 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!

gkalpak Jan 5, 2017 • 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!

petebacondarwin commented Jul 18, 2016

Uh oh!

Narretz commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jan 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jan 31, 2017

Uh oh!

Narretz commented Mar 31, 2017

Uh oh!

Narretz commented Apr 21, 2017

Uh oh!

gkalpak commented Apr 27, 2017

Uh oh!

Narretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gkalpak commented Jul 15, 2016 •

edited

Loading

gkalpak Jan 5, 2017 •

edited

Loading

gkalpak Jan 5, 2017 •

edited

Loading

Narretz commented Jul 21, 2016 •

edited

Loading

gkalpak commented Jan 12, 2017 •

edited

Loading