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

move DOM nodes instead of removing and adding them again when moving an item within it's current list #83

Closed
wants to merge 2 commits into from

Conversation

Crunc
Copy link

@Crunc Crunc commented Jan 28, 2014

First of all thank you for this great knockout binding, it saved me huge amount of time!

Currently DOM nodes are removed and a new DOM node is added, when an item is moved from its position to another position. This caused some trouble because i was doing some fancy layout stuff with the DOM nodes in the sortable list and whenever i moved an item in the list the DOM nodes got recreated and all the fancy stuff i did to them had to be done again. This caused some small performance issues and some severe layout issues due to transitions running again after moving an element and so on.

However i solved it by checking if an item is moved within it's current list. In that case the item is really moved instead of removing and adding it at another position.

Maybe this helps someone who has a similar problem.

Keep on the good work!

best regards,
Crunc

@wellso
Copy link

wellso commented Oct 22, 2015

I tried this and it fixed the issue I was having in #151

@Manuel-S
Copy link
Contributor

that would solve a lot of problems with other components, for example tinymce inside sortable.

@RyanHerbert
Copy link

This fixed an issue I was having also. I had an array of items that was bound to the options of a select box. I was allowing the user to sort the order of the options, and before this patch, moving an item would set the select box back to its default value, instead of retaining its value.

@JD-Robbs
Copy link
Contributor

JD-Robbs commented Mar 1, 2016

I've been seeing the same issue as RyanHerbert - i.e. sorting a list that is also bound to a select input. Currently, I'm having to store any selected items before sorting and then re-select them afterwards (via the provided callbacks).

@JD-Robbs
Copy link
Contributor

Seeing that as per #152 my view model was duplicating some objects at the expense of losing others, I investigated further - including trying out some of the pull requests that have been submitted.

Long story short, the changes included here in #83 definitely solve the duplication issue by virtue of ensuring that DOM nodes are moved rather then plucked-out and then re-inserted.

So I wonder, @rniemeyer , if you think this change, or one to this effect, will be merged?

I think it would add value. Firstly, in instances where moving items deselects currently selected values and, secondly, in instances where, currently, objects are duplicated during the sort operations.

Anyhow, thanks for considering and happy weekend! 👍

@rniemeyer
Copy link
Owner

@JD-Robbs - I originally had a lot of problems with moving rather than replacing items. I definitely would be willing to re-visit this though, as that was when I was originally developing it. I think that it would be best to add an option (could be global + available to override in an individual binding) to turn this on/off (with off being the default for backwards compatibility).

@RyanHerbert
Copy link

If it helps, I've been using this branch in a production application for the past two months without issue.

@wellso
Copy link

wellso commented Mar 29, 2016

@RyanHerbert likewise, been using it for a few months in testing, just about to roll into production

@rniemeyer
Copy link
Owner

OK- I'll see what I can do to get this functionality in with an option to turn it on/off

@JD-Robbs
Copy link
Contributor

I took the liberty to re-appropriate Crunc's work in #160 to implement this as an optional behaviour.

@rniemeyer
Copy link
Owner

This functionality is now in v0.13.0 under the strategyMove option.

@rniemeyer rniemeyer closed this Apr 23, 2016
@Yamo93
Copy link

Yamo93 commented Nov 22, 2019

I tried this and it fixed the issue I was having in #151

I had the same problem and this fixed it!

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

Successfully merging this pull request may close these issues.

8 participants