rniemeyer / knockout-sortable

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

Draggaed DOM element is not removed after sorting inside same array. As result - two exact same DOM elements inside container #112

Closed Kamilius closed 10 years ago

Kamilius commented 10 years ago

Hi.

I have an issue, with dragged object being duplicated inside sortable container after being dropped.

When I'm removing one of those objects with "remove" button on it (which means I'm removing it from observableArray it was rendered by) - one of them disappears and I can't remove the other one (I assume, because only DOM element left like in case with "beforeRemove" if you'll forget to remove element manually, and binded viewmodel was removed, so connection is broken).

I haven't did any fiddles on this, because my situation is like "out of box" with Knockoutjs 3.0.1. I have only one observableArray rendered inside some container with 'sortable', filled with divs as wrappers for items inside this list.

Any ideas on clearing element after dragging?

Edit: after a bit further "Clicking", I've found, that they are not just "duplicating", but sometime without any reaseon - removing some of duplicates.

Edit2: after some more investigation, I've figured out, that for some reason my first item is not being re-rendered(removed) after first "splice" at line 211 (//do actual move) and just jumped to the end of the list, but was added after second splice. That's where I get a duplicate.

Edit3: duplicated DOM element stays inside container, even if observableArray content changed and refreshed a view.

Kamilius commented 10 years ago

Managed to fix this with only by writing my own data-binding (which is certainly easier than going deep into your plugin, especially for my current task). Currently it's aimed only on work within container it was initialized in. I currently don't have a time to adopt it for working with "outer" lists. Hope anyone will find it handy:

ko.bindingHandlers.sortable = {
  init: function(element, valueAccessor, allBindingsAccessor, viewModel, bindingContext) {
    $(element).sortable({
      beforeStop: function(event, ui) {
        var itemElement = ui.item[0],
            itemViewModel = ko.dataFor(itemElement),
            itemIndex = $(element).find('.ui-sortable-placeholder').index() - 1;
        //remove item from list
        viewModel.collectionsList.remove(itemViewModel);
        //remove dom element
        itemElement.remove();
        //insert item into needed place in list
        //so dom rendering will be handled with knockout by itself
        viewModel.collectionsList.splice(itemIndex, 0, itemViewModel);
      }
    });
  }
};
rniemeyer commented 10 years ago

@Kamilius - it would help to see a fiddle of your original scenario, although it sounds like maybe you don't have time. If you do, I would be happy to take a look at it. One issue that I know about is when using name templates. Text nodes around the named template can cause issues. I need to clarify that in the docs. Were you using named templates?

Kamilius commented 10 years ago

@rniemeyer yes, actually I'm using named templates, but in my "afterAdd" function i'm also checking element.nodeType to equal '1', because of this 'text' issue reason, as this value stands for "Element". As for feddle, I can try to reproduce this only at the beginning of the next week.

rniemeyer commented 10 years ago

OK- make sure that your template doesn't have whitespace nodes around it like:

<script id='ryanTmpl' type='text/html'><div>test</div></script>

instead of:

<script id='ryanTmpl' type='text/html'>
    <div>test</div>
</script>
projecteon commented 10 years ago

How to you propse to handle large templates without whitespace? Im running into this issue as well and the sample code does have whitespaces.

rniemeyer commented 10 years ago

@projecteon the template itself can contain whitespace, it is just the top-level nodes that can't. I really would like to find a way to "fix" this in the lib, but will look at adding notes to the docs.

So, you can still do:

<script id="ryanTmpl" type="text/html"><div>
    <ul>
          <li>one</li>
    </ul>
</div></script>
nbsynch commented 10 years ago

This didn't always break though, previously this was working. Do you suspect this was caused by changes within knockout-sortable or something new in the knockout 3.x releases?

rniemeyer commented 10 years ago

The latest release (0.9.0) strips leading/trailing whitespace from named templates to help alleviate this type of issue.

tiltsoftware commented 10 years ago

Hi there, any chance you could update the nuget package to the latest version. I'm having similar issues so will get the latest version from here but would be great to have in nuget so I don't accidentely overwrite it again. Thanks. Love your work.

rniemeyer commented 10 years ago

@tiltsoftware - the Nuget package has been updated. Sorry for the delay.