Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How should foreach binding deal with items mid-animation when updating afterwards #1256

Merged
merged 1 commit into from Jun 24, 2015

Conversation

mbest
Copy link
Member

@mbest mbest commented Jun 15, 2015

From: http://stackoverflow.com/q/14828688/1287183

When adding an item while the beforeRemove animation is running, the removed element is moved to the bottom of the list instead of staying in its position until the animation has finished and the element is removed.

Fiddle: http://jsfiddle.net/bPP5Q/8/

@mbest
Copy link
Member Author

mbest commented Dec 22, 2013

One option that will at least prevent the strangeness in the example would be to remove extra content on each update. (This made me think of #716.)

@mbest mbest modified the milestones: 3.2.0, 3.1.0 Mar 8, 2014
@WinstonFassett
Copy link

I'm seeing the same issues anytime the array is rerendered (renderTemplateForEach) while beforeRemove transitions are still executing.

I first learned about this from this post in the Greensock forum:
http://forums.greensock.com/topic/9183-animating-with-knockoutjs/
fiddle: http://jsfiddle.net/3ucVg/3/

Next I found issue #943, but it was closed, saying to look at the example at http://knockoutjs.com/examples/animatedTransitions.html

While that example "works", it can also produce this error in a number of ways, as shown in this fiddle: http://jsfiddle.net/WinstonF/8k8V5/2264/ (forked from RPN's fiddle: http://jsfiddle.net/rniemeyer/8k8V5/)

It seems like fixing this would require us to treat deleted items that have a beforeRemove callback more like moved or retained items until they are finally removed from the DOM at the end of the transition.

Any suggestions?

@mbest mbest removed this from the 3.2.0 milestone Jun 25, 2014
@mbest mbest added this to the 3.4.0 milestone Jun 14, 2015
…by beforeRemove should be retained in their present position until actually removed from the DOM.
@mbest
Copy link
Member Author

mbest commented Jun 15, 2015

I've worked on a fix for this problem, which I've attached to this issue. The fixed version can be seen demonstrated here: http://jsfiddle.net/mbest/8k8V5/2734/

@mbest
Copy link
Member Author

mbest commented Jun 16, 2015

The fix is to have the foreach binding continue tracking the "removed" nodes until they are actually removed. This means that the beforeRemove handler must remove all of the nodes in order for the binding to stop tracking them. The original jsFiddle example didn't remove non-element nodes (such as text nodes) and so the binding would track all removed items forever.

@mbest
Copy link
Member Author

mbest commented Jun 18, 2015

@rniemeyer, Can you take a look at this and give me your thoughts?

@rniemeyer
Copy link
Member

@mbest - sure, will probably be early next week, but happy to take a look

rniemeyer added a commit that referenced this pull request Jun 24, 2015
How should foreach binding deal with items mid-animation when updating afterwards
@rniemeyer rniemeyer merged commit 34c599a into master Jun 24, 2015
@rniemeyer rniemeyer deleted the 1256-sequential-beforeRemove branch June 24, 2015 13:33
@rniemeyer
Copy link
Member

@mbest - great fix. works very well.

@mbest
Copy link
Member Author

mbest commented Jun 24, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants