rniemeyer / knockout-sortable

A Knockout.js binding to connect observableArrays with jQuery UI sortable functionality
MIT License
547 stars 128 forks source link

Duplicate items when dropping on nested list #111

Closed categulario closed 9 years ago

categulario commented 10 years ago

That's it, I have a nested set of sortables and I drag an item into some other as child it dupplicates (so I can see two identical items inside), or sometimes the item is inside and when I drag it out of its list it duplicates (leaving a copy inside the original container).

http://jsfiddle.net/Ln9e8/

In the example I reproduce the second case

rniemeyer commented 10 years ago

The arrays that are sortable needs to be observableArrays, so you will need to make the child items property be an observableArray.

categulario commented 10 years ago

I updated the fiddle with obervableArrays, still happening

rniemeyer commented 10 years ago

Can you reproduce in this one: http://jsfiddle.net/rniemeyer/zky2T/

categulario commented 10 years ago

It works, I will try to adapt it to my code and see what happens, thanks

categulario commented 10 years ago

Inspecting both, the observableArray and the DOM I realized that the duplicate element was just html, i.e. the array has only the expected objects. Browsing the code I tracked the bug to the line 196 of knockout-sortable wich cancels the sortable event on the source list, but in my case ui.sortable is null (I don't know why) so the event is not cancelled.

What happens if I replace

$(sourceParent === targetParent ? this : ui.sender).sortable("cancel");

with

$(sourceParent === targetParent ? this : $element).sortable("cancel");

?

btw, while trying to run the tests for this new line of code I ran into some trouble with an unexistent function waits that appears frecuently in the spec.

categulario commented 10 years ago

I recently added a pull request that adapts the tests to jasmine 2.0, only if it is useful

projecteon commented 10 years ago

I ended up with duplicate items in the "dragged too" list with this change.

categulario commented 10 years ago

yep, I realized that my fix doesn't work for all cases so I made a new one.

I replaced these lines

//call cancel on the correct list, so KO can take care of DOM manipulation
if (sourceParent) {
    $(sourceParent === targetParent ? this : ui.sender).sortable("cancel");
}
//for a draggable item just remove the element
else {
    $(el).remove();
}

with this one

$(el).remove();

Based on the assumption that the element should be removed in both cases. It solved the problem for me.

rmaziarka commented 9 years ago

It's also solves problem for me. Strange, that half a year ago it was working well without that fix, and yesterday I launched project from beginning and such problem occurred.

categulario commented 9 years ago

Should I make a pull request with the code?

rniemeyer commented 9 years ago

@developingo - sure - PR sounds fine.

rniemeyer commented 9 years ago

@developingo - do you have a sample/fiddle that I can see the scenario that was failing for you? I just want to make sure that I fully understand the situation. Thanks!

categulario commented 9 years ago

Shure! http://jsfiddle.net/yyLx3t4n/ The same as above, with new instructions

rniemeyer commented 9 years ago

Great! I can reproduce the issue in your fiddle.

rniemeyer commented 9 years ago

@developingo - I made an alternate fix that helps call cancel on the appropriate sortable that addresses the issue. Thanks!

chrisjcalderon commented 6 years ago

Had the same problem described on this thread and the $(el).remove(); code replacement worked for me...