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

fix(ngAnimate): ensure that only string-based addClass/removeClass values are applied #12459

Closed
wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Jul 28, 2015

Closes #12458

@matsko matsko force-pushed the fix_add_remove_class_bugs branch from fe378ea to 9783cee Compare July 28, 2015 18:58
matsko added a commit to matsko/angular.js that referenced this pull request Jul 28, 2015
@petebacondarwin
Copy link
Member

@matsko - looks good but I think the implementation could be DRYer with a helper:

function addRemoveClassesPostDigest(element, add, remove) {
  var data = postDigestQueue.get(element);
  var updated = false;
  var classVal;

  var updateData = function(classes, value) {
    var changed = false;
    if (classes) {
      classes = isString(classes) ? classes.split(' ') :
                isArray(classes) ? classes : [];
      forEach(classes, function(className) {
        if (className) {
          changed = true;
          data[className] = value;
        }
      });
    }
    return changed;
  };

  if (!data) {
    postDigestQueue.put(element, data = {});
    postDigestElements.push(element);
  }

  updated = updateData(add, true);
  updated = updated || updateData(remove, false);


  if (!updated || postDigestElements.length > 1) return;

  ...

@ThomasBurleson
Copy link

@matsko, @petebacondarwin - Love the DRY refactor! Question: no support for functions that return strings?

@matsko matsko force-pushed the fix_add_remove_class_bugs branch from 9783cee to 1003a88 Compare July 29, 2015 04:31
@matsko
Copy link
Contributor Author

matsko commented Jul 29, 2015

@petebacondarwin the code has been updated. Please double check and merge in.

@petebacondarwin
Copy link
Member

LGTM

@petebacondarwin
Copy link
Member

Landed. Thanks @matsko.
Although I did tweak the test because I felt that it was clearer to have two it clauses with no conditional logic than the they clause. Hope you don't mind!

@matsko matsko closed this in 0d6fc2d Jul 29, 2015
@petebacondarwin
Copy link
Member

Doh! I only changes one of the test. Sorry.

@matsko
Copy link
Contributor Author

matsko commented Jul 29, 2015

Thank you sir :)

@matsko matsko deleted the fix_add_remove_class_bugs branch July 29, 2015 16:03
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants