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

ngAnimate: enter() and move() are unable to insert/move the new node to the first position in the parent #4934

Closed
njs50 opened this issue Nov 13, 2013 · 16 comments

Comments

@njs50
Copy link

njs50 commented Nov 13, 2013

ngAnimate.enter and ngAnimate.move takes (element, parent, after)

if after is not specified the element is inserted after the last item in the parent.

thus you can't insert or move anything to position [0]. The first position you can insert into is after position[0] i.e at position[1].

It would make more sense to me if either:

  1. they took the node you wanted to insert before instead of after (it is using inserBefore internally already)

or

  1. if you didn't specify after it inserted/moved to position 0 which is otherwise inaccessable.
@petebacondarwin
Copy link
Member

@njs50 - can you provide a plunker to demonstrate this issue? Is this a regression from a previous version, say a release candidate or 1.1.5?

@njs50
Copy link
Author

njs50 commented Nov 13, 2013

I think this animation code is new with the animation refactor in 1.2.0

here is an example for the animate.enter():

http://plnkr.co/edit/K5vGjG1MnCOwD8YmB8hZ?p=preview

note that once you've added the first item there is no way to add a new item before it.

animate.move() has exactly the same problem (can't move an item to the first position).

@ghost ghost assigned matsko Nov 18, 2013
@matsko
Copy link
Contributor

matsko commented Nov 22, 2013

@petebacondarwin no this isn't a regression. We never thought of having such a feature.

This is more of an API challenge than a feature fix. Perhaps the cleanest way would be if you provide true to after and keep parent there then, by considering after === true within $animate it will know to place it at the top of the list of children.

So:

$animate.enter(child, parent, true); //inserts at top
$animate.move(child, parent, true); //moves child to the top of the list

--- EDIT ---

This can't work at all unfortunately, because only the after element knows where the list starts and ends due to the nature of ngRepeat.

@petebacondarwin @IgorMinar @mhevery what do you guys think of the slight API tweak?

@mhevery
Copy link
Contributor

mhevery commented Nov 25, 2013

When you have a repeater which goes to zero, you still need to know where the repeater is located. Repeater does this by keeping a comment node in the DOM tree where the new nodes will go. When the first node is inserted it is inserted after the existing DOM node, hence the API.

I don't think we should support both APIs, but I would be fine in converting it to insertBefore. But it would be a breaking change.

@matsko
Copy link
Contributor

matsko commented Nov 25, 2013

Having insertBefore doesn't solve this because what happens if you want to append something to the end of the list, but the list itself has some other DOM elements that appear after it?

<div class="my-list-container">

<div>....</div>
<div>....</div>
<div>....</div>

<div class="my-list-item">....</div>
<div class="my-list-item">....</div>
<div class="my-list-item">....</div>

<div>....</div>
<div>....</div>
<div>....</div>

</div>

For the three .my-list-item elements, you can insert a new element before each item. But if you insert inside of the .my-list-container element then it will place the new item after the final three <div>...</div> elements.

Maybe instead you could have:

$animate.enter(element, null, { element : afterNode, before : true });

//or a new $animate method:

$animate.enterBefore(element, null, afterNode);

@njs50
Copy link
Author

njs50 commented Nov 25, 2013

I think the ngAnimation module decorates the core $animate.enter() function. Any change would need to be directly in $animate.enter() for the actual animation module to work without having to make changes in there.

@matsko In that case you could just do what animate.enter() is doing internally at the moment. i.e finding the node after the one you provide and inserting before that.

Either way, given that this is used for all animation and not just for repeaters I think the first node being immutable is not a good long term plan.

@matsko
Copy link
Contributor

matsko commented Dec 16, 2013

This is a tricky approach since we always need to have a reference to the former element even if you are doing before or after enter (you need both operations). Otherwise without the former element, then putting something at the bottom of the list may place it under all of the other elements (see my code example above).

So this means we'll have to change the API for the input call. Which means this is going to go into Angular 1.3.

@dehru
Copy link

dehru commented Dec 23, 2013

+1

@matsko
Copy link
Contributor

matsko commented Dec 30, 2013

This will come through with Angular 1.3.X since it requires a public API change for $animate.move()

@swarajban
Copy link

+1

@matsko
Copy link
Contributor

matsko commented Mar 11, 2014

Now that we're in 1.3 land we can think of an API change for this.

@matsko
Copy link
Contributor

matsko commented Mar 11, 2014

Sorry I made the solution more complex than what it is. We just need to change src/ng/animate.js to insert at the top of the list (as the first node of the element) instead of at the bottom.

@njs50
Copy link
Author

njs50 commented Mar 11, 2014

so when no 'after' node is specified it would insert at the top (as first element of parent)? I think that would involve the least amount of change to get everything working.

The breaking change would then be anyone who hadn't specified an after node but wanted to insert at the end of the list. In that case they'd need to find the last node in the parent and specify that as the after node.

that sounds like a good plan to me?

@matsko
Copy link
Contributor

matsko commented Mar 11, 2014

Yes. That's the plan. 1.3 can include breaking changes since the API is not stable yet.

@thebigredgeek
Copy link
Contributor

@matsko I have never enjoyed hearing that....

matsko added a commit to matsko/angular.js that referenced this issue Mar 12, 2014
…nstead of at the end

BREAKING CHANGE: $animate will no longer place elements at the end of the parent container
when enter or move is used with a parent container element. Any former directives that rely
on this feature must be slightly modified to work around this fix. The same effect can be achieved
by finding the final element in the parent container list and using that as the after element
(which is the 3rd argument in the enter and move $animate operations).

Closes angular#4934
@matsko
Copy link
Contributor

matsko commented Mar 12, 2014

PR is ready: #6663

matsko added a commit to matsko/angular.js that referenced this issue Apr 1, 2014
…nstead of at the end

BREAKING CHANGE: $animate will no longer place elements at the end of the parent container
when enter or move is used with a parent container element. Any former directives that rely
on this feature must be slightly modified to work around this fix. The same effect can be achieved
by finding the final element in the parent container list and using that as the after element
(which is the 3rd argument in the enter and move $animate operations).

Closes angular#4934
Closes angular#6275
matsko added a commit to matsko/angular.js that referenced this issue Apr 1, 2014
…nstead of at the end

$animate.enter and $animate.move both insert the element at the end of the provided parent
container element. If an after element is provided then they will place the inserted element
after that one. This works fine, but there is no way to place an item at the top of the provided
parent container using both these functions. This commit fixes this issue.

BREAKING CHANGE: $animate will no longer place elements at the end of the parent container
when enter or move is used with a parent container element. Any former directives that rely
on this feature must be slightly modified to work around this fix. The same effect can be achieved
by finding the final element in the parent container list and using that as the after element
(which is the 3rd argument in the enter and move $animate operations).

Closes angular#4934
Closes angular#6275
matsko added a commit to matsko/angular.js that referenced this issue Apr 3, 2014
…nstead of at the end

With 1.2.x, $animate.enter and $animate.move both insert the element at the end of the provided
parent container element when only the parent element is provided. If an after element is provided
then they will place the inserted element after that one. This works fine, but there is no way to
place an item at the top of the provided parent container using both these functions.

Closes angular#4934
Closes angular#6275

BREAKING CHANGE: $animate will no longer default the after parameter to the last element of the parent
container. Instead, when after is not specified, the new element will be inserted as the first child of
the parent container.

To update existing code change any instances of $animate.enter() or $animate.move() from:

$animate.enter(element, parent);

to:

$animate.enter(element, parent, angular.element(parent[0].lastChild));
matsko added a commit to matsko/angular.js that referenced this issue Apr 3, 2014
…nstead of at the end

With 1.2.x, $animate.enter and $animate.move both insert the element at the end of the provided
parent container element when only the parent element is provided. If an after element is provided
then they will place the inserted element after that one. This works fine, but there is no way to
place an item at the top of the provided parent container using both these functions.

Closes angular#4934
Closes angular#6275

BREAKING CHANGE: $animate will no longer default the after parameter to the last element of the parent
container. Instead, when after is not specified, the new element will be inserted as the first child of
the parent container.

To update existing code change any instances of $animate.enter() or $animate.move() from:

`$animate.enter(element, parent);`

to:

`$animate.enter(element, parent, angular.element(parent[0].lastChild));`
@matsko matsko closed this as completed in 1cb8584 Apr 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.