Skip to content
This repository was 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
matsko wants to merge 1 commit into
angular:masterfrom
matsko:fix_add_remove_class_bugs
Closed

matsko wants to merge 1 commit into
angular:masterfrom
matsko:fix_add_remove_class_bugs

Conversation

@matsko

@matsko matsko commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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
Copy Markdown

@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

matsko commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

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

@petebacondarwin

Copy link
Copy Markdown
Contributor

LGTM

@petebacondarwin

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@matsko

matsko commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

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.

4 participants