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

fix(ngRepeat): trigger move animation #15072

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

fix(ngRepeat): trigger move animation #15072

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2331c42
fix(ngRepeat): trigger move animation
pondermatic Aug 30, 2016
263be6b
style(ngRepeat) eslint does not like unused functions and requires co…
pondermatic Aug 30, 2016
f3c6062
style(ngRepeat): getBlockStart function is no longer needed
pondermatic Sep 1, 2016
47bd3a4
style(ngRepeat): kick off Travis-CI
pondermatic Sep 1, 2016
7a98172
perf(ngRepeat): use less memory and compare less
pondermatic Sep 12, 2016
d8320a1
Merge branch 'ngRepeat-animations' of https://github.com/pondermatic/…
pondermatic Sep 12, 2016
dcc80c1
Merge branch 'master' into ngRepeat-animations
pondermatic Mar 21, 2017
c969ae2
fix(ngRepeat): correctly re-create last block order when track by is …
pondermatic Mar 21, 2017
410312c
test(ngRepeat): Update based on feedback.
pondermatic Mar 24, 2017
4d6feb9
test(ngRepeat): Update based on feedback.
pondermatic Mar 25, 2017
3b17d5d
test(ngRepeat): Add 'toContainQueueItem' Jasmine matcher
pondermatic Mar 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
style(ngRepeat) eslint does not like unused functions and requires co… 
…mparison operators with type.
  • Loading branch information
@pondermatic
pondermatic committed Aug 30, 2016
commit 263be6bbdfe67a877c50430fcb4acd9045ff8448
6 changes: 3 additions & 3 deletions src/ng/directive/ngRepeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,9 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani
scope.$odd = !(scope.$even = (index & 1) === 0);
};

var getBlockStart = function(block) {
return block.clone[0];
};
// var getBlockStart = function(block) {
// return block.clone[0];
// };
Copy link
Contributor

@austinoneil austinoneil Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove this commented-out code?


Copy link
Author

@pondermatic pondermatic Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBlockStart function is no longer used.

var getBlockEnd = function(block) {
return block.clone[block.clone.length - 1];
Expand Down
2 changes: 1 addition & 1 deletion test/ng/directive/ngRepeatSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,7 +1489,7 @@ describe('ngRepeat animations', function() {

while ($animate.queue.length) {
item = $animate.queue.shift();
if (item.event == 'leave') {
if (item.event === 'leave') {
break;
}
}
Copy link
Contributor

@petebacondarwin petebacondarwin Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment to explain why this is needed would be helpful.

Copy link
Author

@pondermatic pondermatic Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to revert this code. The ngRepeat tests assume the animation queue is in a certain order. During refactoring of ngRepeat, the order was different in this test so I altered the test. My latest version of ngRepeat, c969ae2, passes the original version of this test.

Copy link
Author

@pondermatic pondermatic Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should tests assume that animation events are queued in a certain order? If not, then perhaps the queue should be tested like this:

// Test each queue item's event type
while ($animate.queue.length) {
  item = $animate.queue.shift();
  switch (item.element.text()) {
    case '2':
      expect(item.event).toBe('leave');
      break;
    case '3':
      expect(item.event).toBe('move');
      break;
    default:
      expect(item).toBeUndefined();
  }
}

Maybe we should add a Jasmine matcher in the "ngRepeat Animations" block that compares item.event and item.text() values between two arrays? Should this be new GitHub issue?

Copy link
Author

@pondermatic pondermatic Mar 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, the animation queue order IS different than it used to be.
Options:

  1. use a while loop to search for the leave event, as in commit 2331c42
  2. shift the queue array a second time, assuming that the leave event is the second item in the queue
  3. add a Jasmine matcher that searches the queue for a item with a specific element text and event

As far as I can tell, tests should not assume the order of events in the queue. So I propose the following matcher be added to the 'ngRepeat animations' group of tests:

beforeEach(function() {
  jasmine.addMatchers({
    toContainQueueItem: function() {
      return {
        compare: generateCompare(false),
        negativeCompare: generateCompare(true)
      };
      function generateCompare(isNot) {
        /**
         * Matcher that checks that the expected element text is in the actual Array of
         * animation queue items and that the event matches.
         * @param {array} actual
         * @param {string} expectedElementText
         * @param {string} expectedEvent optional if isNot = true
         * @returns {{pass: boolean, message: message}}
         */
        return function(actual, expectedElementText, expectedEvent) {
          if (expectedEvent === undefined) {
            expectedEvent = '';
          }
          var actualLength = actual.length;
          var i;
          var item;
          var message = valueFn(
            'Expected animation queue to ' + (isNot ? 'not ' : '') + 'contain an item where the element\'s text is "'
            + expectedElementText + '"' + (isNot ? '' : ' and the event is "' + expectedEvent + '"')
          );
          var pass = isNot;

          for (i = 0; i < actualLength; i++) {
            item = actual[i];
            if (item.element.text() === expectedElementText) {
              pass = item.event === expectedEvent;
              break;
            }
          }

          return {'pass': pass, 'message': message};
        };
      }
    }
  });
});

The tests should use the matcher like this:

expect($animate.queue).not.toContainQueueItem('1');
expect($animate.queue).toContainQueueItem('2', 'leave');
expect($animate.queue).toContainQueueItem('3', 'move');
$animate.queue = [];

What does everyone think?

Expand Down