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

fix(ngRepeat): trigger move animation #15072

Open
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 all commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
160 changes: 76 additions & 84 deletions src/ng/directive/ngRepeat.js
View file
Open in desktop
Expand Up @@ -338,7 +338,6 @@
</example>
*/
var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $animate, $compile) {
var NG_REMOVED = '$$NG_REMOVED';
var ngRepeatMinErr = minErr('ngRepeat');

var updateScope = function(scope, index, valueIdentifier, value, keyIdentifier, key, arrayLength) {
Expand All @@ -353,15 +352,10 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani
scope.$odd = !(scope.$even = (index & 1) === 0);
};

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

var getBlockEnd = function(block) {
return block.clone[block.clone.length - 1];
};


return {
Copy link
Author

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.

restrict: 'A',
multiElement: true,
Expand Down Expand Up @@ -438,126 +432,124 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani

//watch props
$scope.$watchCollection(rhs, function ngRepeatAction(collection) {
var index, length,
previousNode = $element[0], // node that cloned nodes should be inserted after
// initialized to the comment node anchor
nextNode,
// Same as lastBlockMap but it has the current state. It will become the
// lastBlockMap on the next iteration.
nextBlockMap = createMap(),
collectionLength,
key, value, // key/value of iteration
trackById,
trackByIdFn,
collectionKeys,
block, // last object information {scope, element, id}
nextBlockOrder,
elementsToRemove;
var
block, // last object information {scope, element, id}
collectionIsLikeArray,
collectionKeys = [],
elementsToRemove,
index, key, value,
lastBlockOrder = [],
lastKey,
nextBlockMap = createMap(),
nextBlockOrder = [],
nextKey,
nextLength,
previousNode = $element[0], // node that cloned nodes should be inserted after
// initialized to the comment node anchor
trackById,
trackByIdFn;
Copy link
Author

Choose a reason for hiding this comment

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

variables are defined in alphabetical order (mostly)


if (aliasAs) {
$scope[aliasAs] = collection;
}

if (isArrayLike(collection)) {
collectionKeys = collection;
// get collectionKeys
collectionIsLikeArray = isArrayLike(collection);
Copy link
Author

Choose a reason for hiding this comment

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

It is faster to check the collectionIsLikeArray boolean than to compare value and type of collectionKeys and collection inside a loop.

if (collectionIsLikeArray) {
trackByIdFn = trackByIdExpFn || trackByIdArrayFn;
} else {
trackByIdFn = trackByIdExpFn || trackByIdObjFn;
// if object, extract keys, in enumeration order, unsorted
collectionKeys = [];
for (var itemKey in collection) {
if (hasOwnProperty.call(collection, itemKey) && itemKey.charAt(0) !== '$') {
collectionKeys.push(itemKey);
for (key in collection) {
if (hasOwnProperty.call(collection, key) && key.charAt(0) !== '$') {
collectionKeys.push(key);
}
}
}
nextLength = collectionIsLikeArray ? collection.length : collectionKeys.length;

collectionLength = collectionKeys.length;
nextBlockOrder = new Array(collectionLength);

// locate existing items
for (index = 0; index < collectionLength; index++) {
key = (collection === collectionKeys) ? index : collectionKeys[index];
// setup nextBlockMap
Copy link
Author

Choose a reason for hiding this comment

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

The "setup nextBlockMap" loop has a similar purpose to the "locate existing items" loop in the previous version. It does not delete previously seen blocks from lastBlockMap. Therefore, lastBlockMap does not need to be restored if a duplicate block is detected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation of the comment here is wrong

Copy link
Member

Choose a reason for hiding this comment

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

@Narretz Is it? Looks good to me.

for (index = 0; index < nextLength; index++) {
key = collectionIsLikeArray ? index : collectionKeys[index];
value = collection[key];
trackById = trackByIdFn(key, value, index);
if (lastBlockMap[trackById]) {
// found previously seen block
block = lastBlockMap[trackById];
delete lastBlockMap[trackById];
nextBlockMap[trackById] = block;
nextBlockOrder[index] = block;
} else if (nextBlockMap[trackById]) {
// if collision detected. restore lastBlockMap and throw an error
forEach(nextBlockOrder, function(block) {
if (block && block.scope) lastBlockMap[block.id] = block;
});

if (nextBlockMap[trackById]) {
// if collision detected, throw an error
throw ngRepeatMinErr('dupes',
'Duplicates in a repeater are not allowed. Use \'track by\' expression to specify unique keys. Repeater: {0}, Duplicate key: {1}, Duplicate value: {2}',
expression, trackById, value);
} else {
// new never before seen block
nextBlockOrder[index] = {id: trackById, scope: undefined, clone: undefined};
nextBlockMap[trackById] = true;
'Duplicates in a repeater are not allowed. Use \'track by\' expression to specify unique keys. Repeater: {0}, Duplicate key: {1}, Duplicate value: {2}',
expression, trackById, value);
}
}

// remove leftover items
for (var blockKey in lastBlockMap) {
block = lastBlockMap[blockKey];
elementsToRemove = getBlockNodes(block.clone);
$animate.leave(elementsToRemove);
if (elementsToRemove[0].parentNode) {
// if the element was not removed yet because of pending animation, mark it as deleted
// so that we can ignore it later
for (index = 0, length = elementsToRemove.length; index < length; index++) {
elementsToRemove[index][NG_REMOVED] = true;
}
}
block.scope.$destroy();
nextBlockMap[trackById] = {id: trackById, clone: undefined, scope: undefined, index: index};
nextBlockOrder[index] = trackById;
}

// we are not using forEach for perf reasons (trying to avoid #call)
for (index = 0; index < collectionLength; index++) {
key = (collection === collectionKeys) ? index : collectionKeys[index];
value = collection[key];
block = nextBlockOrder[index];
// Set up lastBlockOrder. Used to determine if a block moved.
for (key in lastBlockMap) {
lastBlockOrder[lastBlockMap[key].index] = key;
}

if (block.scope) {
// if we have already seen this object, then we need to reuse the
// associated scope/element
for (index = 0; index < nextLength; index++) {
key = collectionIsLikeArray ? index : collectionKeys[index];
nextKey = nextBlockOrder[index];

nextNode = previousNode;
if (lastBlockMap[nextKey]) {
// we have already seen this object and need to reuse the associated scope/element
block = lastBlockMap[nextKey];

// skip nodes that are already pending removal via leave animation
do {
nextNode = nextNode.nextSibling;
} while (nextNode && nextNode[NG_REMOVED]);
// move
Copy link
Author

Choose a reason for hiding this comment

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

The logic of when to move and where to move is a key change in this pull request.

if (lastBlockMap[nextKey].index !== nextBlockMap[nextKey].index) {
// If this block has moved because the last previous block was removed,
// then use the last previous block to set previousNode.
Copy link
Author

Choose a reason for hiding this comment

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

This loop removes elements and now occurs after the "enter and move" loop.

lastKey = lastBlockOrder[lastBlockMap[nextKey].index - 1];
if (lastKey && !nextBlockMap[lastKey]) {
previousNode = getBlockEnd(lastBlockMap[lastKey]);
}

if (getBlockStart(block) !== nextNode) {
// existing item which got moved
$animate.move(getBlockNodes(block.clone), null, previousNode);
block.index = nextBlockMap[nextKey].index;
}

updateScope(block.scope, index, valueIdentifier, collection[key], keyIdentifier, key, nextLength);

nextBlockMap[nextKey] = block;
previousNode = getBlockEnd(block);
updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);

} else {
// enter
// new item which we don't know about
$transclude(function ngRepeatTransclude(clone, scope) {
block.scope = scope;
nextBlockMap[nextKey].scope = scope;
// http://jsperf.com/clone-vs-createcomment
var endNode = ngRepeatEndComment.cloneNode(false);
clone[clone.length++] = endNode;

$animate.enter(clone, null, previousNode);
previousNode = endNode;

// Note: We only need the first/last node of the cloned nodes.
// However, we need to keep the reference to the jqlite wrapper as it might be changed later
// by a directive with templateUrl when its template arrives.
block.clone = clone;
nextBlockMap[block.id] = block;
updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
nextBlockMap[nextKey].clone = clone;
updateScope(scope, nextBlockMap[nextKey].index, valueIdentifier, collection[key], keyIdentifier, key, nextLength);
});
}
}

// leave
// This must go after enter and move because leave prevents getting element's parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this further? The leave animation section seems to have changed quite a bit from the original. Is this necessary for this PR?

Copy link
Author

Choose a reason for hiding this comment

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

The main change is there no need to keep track of deleted elements because we aren't doing any more processing where we might need to ignore the element. The other change is to skip blocks that don't need removed.

for (key in lastBlockMap) {
if (nextBlockMap[key]) {
continue;
}

block = lastBlockMap[key];
elementsToRemove = getBlockNodes(block.clone);
$animate.leave(elementsToRemove);
block.scope.$destroy();
}

lastBlockMap = nextBlockMap;
});
};
Expand Down