rniemeyer / knockout-sortable

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

Added Droppable binding #180

Closed rposener closed 7 years ago

rposener commented 7 years ago

I know there is another pending droppable binding pull request, but thought this a simpler, potentially less contentious version. Would love to see this added, as we use this library heavily in a few areas, and having this functionality is quite nice.

rniemeyer commented 7 years ago

@rposener - This looks good. This is quite similar to how I have done droppable in the past (basically give it an observable to update with the dropped value). I always thought it worked out well and is simple to understand. Would you be willing to attempt some tests and update the README?

rposener commented 7 years ago

Sure thing. Will try to bang out a few tests right now and give a quick update on the readme.

bago commented 7 years ago

Hi all, I also developed a droppable for knockout-sortable in my mosaico email editor project: https://github.com/voidlabs/mosaico/blob/master/src/js/bindings/droppable.js

My version supports the isEnabled and clone property and the dragged callback. It is used live in mosaico since 18 months with no issues (but this is only a specific use case).

I'm sorry I never provided a PR but there was already another "controversial" PR on this and I didn't find the "time" to do that.

If you want to use my code or part of it, feel free to just use it under the MIT licensing terms (only that file.. while mosaico itself is GPL).

rposener commented 7 years ago

I updated this a little more to support passing all options along. I updated the readme, and added 3 very basic tests - droppable doesn't really do much but pass options, and wire the drop event. @bago if you find that it's missing anything your other binding had, please let me know.

bago commented 7 years ago

@rposener my binding supported the clone and dragged properties as a mirror of what sortable supports when receiving objects from a draggable: https://github.com/rniemeyer/knockout-sortable/blob/master/src/knockout-sortable.js#L195

rniemeyer commented 7 years ago

@rposener - I like the implementation in this PR at the moment. Thank-you for adding the tests/docs. I'd like to work to get this merged and we can evaluate for future whether we need some of the additional options from the nice binding that @bago linked.

bago commented 7 years ago

OK. I just propose to at least rename "drop" to "data" like in current draggable: https://github.com/rniemeyer/knockout-sortable/blob/master/src/knockout-sortable.js#L400

In order to use KO-sortable in mosaico I also need the other bindings, so I will keep my own extension (even if I don't like too much that my extension will override an existing extension once I upgrade) unless the other options are introduced too.

rniemeyer commented 7 years ago

@bago @rposener - changing drop to data is a good change

rposener commented 7 years ago

I made that change in my repository.

rposener commented 7 years ago

I pulled into my project and immediately found that the AMD dependencies were not listed. Updated for that.

When you merge, can we get an updated nuget package as well? We have some older projects still using nuget for our js libraries.

rniemeyer commented 7 years ago

@rposener - This is merged. Thanks for the contribution! I had to make a few tweaks to support passing just an observable (ensure that value is not the unwrapped version of it) and to support dropping a sortable item onto a droppable (look for data on ITEMKEY as well as DRAGKEY).

If you have time, I'd be happy to include a demo of using the droppable binding with the others in the examples folder.

rniemeyer commented 7 years ago

@rposener - Also I'll try to get the Nuget package updated as soon as I can (having a problem with the VM that I have that setup on).

rposener commented 7 years ago

I will try to get an example added and send another PR. Thanks!