Support more elements inside the directive, not just ngRepeat

[romario333]

Previous Title: "Empty item is inserted to array after reorder if there are non-sortable siblings"

If you setup ui-sortable to sort only some elements in the container (e.g. by configuring sortable with items: '> .sortable') and keep in sync with model via ng-model, there is a risk that new empty items will be inserted into the array.

Have a look at demo here. Grab the second item and put it before the first. The new empty item is created.

[thgreasi]

You shouldn't place 'extra' items inside the sortable directive. This is because jquery.ui.sortable is going to report invalid item indexes about the node dragged.
I would suggest to embrace them (those placeholder items) as a type of item inside your model. This fells more like the 'angular way' to me. In more complex scenarios you can even load templates dynamically.

Here is a proof of concept of my suggestion.

[romario333]

Yes, jquery.ui.sortable reports invalid item index in this case. This is a bug. It should be either fixed or you shouldn't be able to configure the underlying jQuery plugin to do this (or at least it should warn you).

As to your second point: having these placeholders in the model is just a workaround. It doesn't feel like the 'angular way' to me. Consider this scenario: you have a list of items, first item represents Add button. If I put fake entry representing Add button to the model, I have to add logic which removes it from the model before I send it to server. Also I still need to have logic in HTML, because Add button needs at minimum click handler. Too much code for such a simple thing.

[thgreasi]

If the items you are repeating & sorting are not buttons as well, then I think that it would be more semantic (and easy) to place the 'Add Button' on a different level in the Dom.

This is just the way this directive was designed to work, to achieve good performance and slim/clean/maintainable code.

What if someone needed multiple repeaters inside the directive? Should that also be implemented?

On the other hand, if you open a Pull Request with a suggested solution, I will be happy to review it and merge it.

[romario333]

Hi, I have played with this a little and I have an idea how to fix this. Check out https://github.com/romario333/ui-sortable/blob/issue-41/test/issue-41.html to see the fix in action.

The basic idea behind the fix is this: To get the index of dragged element I'm going to ask Sortable widget to give me the collection of all items it tracks. Then I simply return the index of dragged element within this collection.

There are some complications though:

1. I don't see any public method in Sortable widget which would return elements tracked by Sortable. However toArray is pretty close. So for now I have created very similar function getItems and I'm monkey-patching it on Sortable.
2. In issue-41.html I'm using items option to limit which elements will be sortable. The problem is that if you don't pass this option during the first call to sortable(), it does not work correctly (I haven't investigated why). For now as a workaround I'm adding scope.$eval(attrs.uiSortable) to initial options.

This is obviously just a proof of concept, it needs additional work. Point 1 can be solved pretty easily by putting it directly into the Sortable widget (which I can do, however I haven't contributed to jQuery UI before so I'm not sure what it takes).

As to the point 2, it seems like a jQuery UI's bug, I'll investigate it when I have time.

[romario333]

I have updated the original plunk example to show the fix in action.

[thgreasi]

Please also note that a use case like this also exists.
In this use case, the model provides a way to determine which elements will be sortable.

If i am getting this right, in the proof of concept implementation, you are checking whether the item is sortable to determine if it is generated from the $scope.model. This is not an ideal assumption and produces strange results for the above use case (try moving 'item 1' below 'item 6').

[thgreasi]

Renaming issue
from: "Empty item is inserted to array after reorder if there are non-sortable siblings"
to: "Support more elements inside the directive, not just ngRepeat"
to indicate that this is not a case of the suggested setup.