search

rniemeyer / knockout-sortable

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

Knockout 3.5.0 upgrade issue #202

Closed jzs11 closed 4 years ago

jzs11 commented 4 years ago

@rniemeyer could you please review the code change? Thanks

rniemeyer commented 4 years ago

@jingzheshan - hello! Thanks for the PR. I think that I would go with something like I had here: https://github.com/rniemeyer/knockout-sortable/commit/3033da209a07b7b499777d642bbc2b0dbb997216 from this branch: https://github.com/rniemeyer/knockout-sortable/commits/defect-with-ko-3-5-0 , although your code could be adapted pretty easily. The main thing is that even if no options are passed (sortable: items), the "global" options from ko.bindingHandlers.sortable.* still need to be applied. Thanks.

jzs11 commented 4 years ago

@jingzheshan - hello! Thanks for the PR. I think that I would go with something like I had here: 3033da2 from this branch: https://github.com/rniemeyer/knockout-sortable/commits/defect-with-ko-3-5-0 , although your code could be adapted pretty easily. The main thing is that even if no options are passed (sortable: items), the "global" options from ko.bindingHandlers.sortable.* still need to be applied. Thanks.

@rniemeyer thanks for the feedback, I tried the one you had before, however it has an issue if the options is a complex type, it will prevent the evaluation and leads to UI stop updating.

And yea, I missed the no options are passed part 😅

rniemeyer commented 4 years ago

@jingzheshan - can you share with me a sample of the binding/code that was failing with my original fix? Maybe we can get it working in either version of the fix and go from there.

jzs11 commented 4 years ago

@rniemeyer thanks for the response. The correct behavior with the code from this pull request works

The unexpected behavior with the code from https://github.com/rniemeyer/knockout-sortable/commit/3033da209a07b7b499777d642bbc2b0dbb997216 non-works

The binding information: image

Sry just providing the screenshots, maybe I should build a js fiddle for it, been lazy 😋

rniemeyer commented 4 years ago

@jingzheshan - there is a version 1.2.0 that addresses 3.5.0 and 3.5.1 compatibility issues. Let me know if you are still experiencing any issues with it. Thank you for providing code/feedback on the issues.