rniemeyer / knockout-sortable

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

isEnabled/allowDrop don't update when set by a computed expression #38

Open jabberwik opened 11 years ago

jabberwik commented 11 years ago

I have a <div> that shows one of several sortable lists, selectable from a drop-down. Some of these lists are read-only. My binding looks like this:

<div class="board" data-bind="css: { readonly: readOnly }>
    <select data-bind="options: lists"></select>
    <div class="board-body" data-bind="sortable: { template: 'listItemTmpl', data: items, isEnabled: !readOnly(), allowDrop: !readOnly() }">
    </div>
</div>

The property readOnly is an observable and updates when lists is changed. If the user changes from a read-only list to a writeable one, the binding doesn't update the isEnabled or allowDrop behaviors.

My current workaround is to use a computed observable writeable that just returns !readOnly(), and this works as expected. So it's not show-stopping, but every other binding lets you use expressions at any point, so this should probably be made consistent.

rniemeyer commented 11 years ago

Hello- This is an unfortunate by product of the way that the binding is designed. It uses computed observables that target the specific options, so if the value has already been unwrapped (an expression in the binding string) before the computed is setup, then the computed will not gain a dependency on the observable, as it will only receive the already calculated value.

It is not an easy one to handle. The choices are to unwrap all of the options in the "update" function and try to make sense of which option was actually being triggered (gets kind of messy) or split out the options into separate bindings (sortableAllowDrop and sortableIsEnabled). This might be a decent option, although it would likely require quite a bit of extra code, just to support this scenario.

I will think about it. I understand that the behavior is not obvious or necessarily consistent with what people are used to with simpler bindings.

mbest commented 11 years ago

This is related to https://github.com/SteveSanderson/knockout/issues/500, which hopefully we can fix in the next version of Knockout.

Edit: Okay, it's slightly related, but even if the issue was fixed, the sortable binding wouldn't be able to individually subscribe to updates to isEnabled and allowDrop. It looks like you can do something like this though: isEnabled: function() { return !readOnly() }, allowDrop: function() { return !readOnly() }, and that's something that could possibly be added automatically with a future version of Knockout.