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

$animate tallying should account for no-ops #8946

Closed
jonrimmer opened this issue Sep 5, 2014 · 4 comments
Closed

$animate tallying should account for no-ops #8946

jonrimmer opened this issue Sep 5, 2014 · 4 comments

Comments

@jonrimmer
Copy link

So if I have a directive that does the following...

scope.$watch('property', function(value) {
  if (value) {
    $animate.addClass(element, 'has-value');
  }
  else {
    $animate.removeClass(element, 'has-value');
  }
});

...and during the initial digest, the value of property is initially null, but is then populated from some data store. Then the watch will be called twice, resulting in a removeClass() call (when the class isn't actually present), followed by an addClass() call. The end result would be the class is not added, which is not what I want.

Prior to to RC0, this pattern was simple, obvious, and it worked. It's a little wasteful to have the redundant removeClass() call, but since the class isn't actually present at that point, it's basically a no-op.

In RC0, $animate's tallying breaks this pattern. I presume it is counting the two calls as (-1) + 1, equalling 0, so performing no animation. However, since the the first removeClass() call is a no-op, the subsequent addClass() call should still run.

More generally, any call to addClass() and removeClass() that wouldn't actually result in a change should not be considered for tallying. If, during a digest cycle, the following series of operations was performed:

removeClass() -> No-op
addClass()
addClass() -> No-op
addClass() -> No-op
removeClass()

It should be logically equivalent to the following:

addClass()
removeClass()

With the result being that no class is added.

Without this, the developer is forced to manually do the same de-duping everywhere such class toggling is applied using an extra state variable.

@petebacondarwin
Copy link
Contributor

I suspect that there is some reasoning behind such tallying. Probably to do with ngAnimate not being able to be sure what order the calls are made. Without the tallying you would get the last in wins situation, which I guess is undesirable.

In your case, unless undefined really is a valid value for value then I would do:

scope.$watch('property', function(value) {
  if (angular.isDefined(value)) {
    if (value) {
      $animate.addClass(element, 'has-value');
    }
    else {
      $animate.removeClass(element, 'has-value');
    }
  }
});

By the way, all watchers are called at start-up with undefined for the value to initialize them. So this is a common pattern.

@petebacondarwin
Copy link
Contributor

Perhaps @matsko has some view on this...

@IgorMinar
Copy link
Contributor

I spoke with @matsko and he likes the proposal.

@matsko
Copy link
Contributor

matsko commented Oct 4, 2014

Working on this on Sunday.

matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the class times that the classes were added and/or removed.

Closes angular#8946
matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent class times that the classes were added and/or removed
and it will only remember a total of one addClass or removeClass operation per
className.

```pre
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +1
removeClass => 0
// this will cause no animation to run
```

Closes angular#8946
matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent class times that the classes were added and/or removed
and it will only remember a total of one addClass or removeClass operation per
className.

```
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +1
removeClass => 0
// this will cause no animation to run
```

Closes angular#8946
matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent class times that the classes were added and/or removed
and it will only remember a total of one addClass or removeClass operation per
className.

```
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +1
removeClass => 0
// this will cause no animation to run
```

Closes angular#8946
matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent class times that the classes were added and/or removed
and it will only remember a total of one addClass or removeClass operation per
className.

```
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +1
removeClass => 0
// this will cause no animation to run
```

Closes angular#8946
matsko added a commit to matsko/angular.js that referenced this issue Oct 7, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent addClass or removeClass operation and will only perform
that operation (depending on what was last called).

```
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => add
removeClass => remove
addClass    => add
addClass    => add
removeClass => remove
// this will cause a rmeoveClass animation
```

Closes angular#8946
@matsko matsko closed this as completed in c93924e Oct 7, 2014
bullgare pushed a commit to bullgare/angular.js that referenced this issue Oct 9, 2014
…ost recent DOM operations

Prior to this fix $animate would maintain a count of each time a class was
added and removed within $animate. With this fix, $animate instead only cares
about the most recent addClass or removeClass operation and will only perform
that operation (depending on what was last called).

```
// before
addClass    => +1
removeClass => 0
addClass    => +1
addClass    => +2
removeClass => +1
// this will cause an addClass animation

// now
addClass    => add
removeClass => remove
addClass    => add
addClass    => add
removeClass => remove
// this will cause a removeClass animation
```

Closes angular#8946
Closes angular#9458
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.