rniemeyer / knockout-sortable

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

allowDrop not working as expected #159

Closed maz4focus closed 8 years ago

maz4focus commented 8 years ago

allowDrop may work fine with simple examples, where only the len of the array is checked. However, if you do a check that tests the content of the array, drop is allowed regardless of what allowDrop is returning. To achieve the goal, I had to provide an beforeMove function that will set cancelDrop to true if the condition is not met. Luckily, beforeMove is getting all needed information so this can be achieved easily. However, I find allowDrop's behaviour misleading, confusing and wrong. I find that allowDrop should be a function that gets all needed info (e.g. the item to be tested and the target array) instead of a function that gets nothing. And it should not be invoked AFTER the array is already modified; this is not clear to me. In my opinion, it should be invoked BEFORE the item is inserted into the target array, and if you return false the item should NOT be inserted. I just realized that actually that's what beforeMove does. But then, why is there allowDrop?

rniemeyer commented 8 years ago

allowDrop makes it so the list is not a target to drop on at all where beforeMove lets you run logic when a drop happens and can allow you to choose to cancel that drop.