search

gvas / knockout-jqueryui

Knockout bindings for the jQuery UI widgets.
http://gvas.github.com/knockout-jqueryui/
MIT License
103 stars 38 forks source link

Bidirectional 'values' binding for slider #35

Closed manuel-guilbault closed 10 years ago

manuel-guilbault commented 10 years ago

Made the values binding bidirectional for the slider widget.

Also added a new option called realtime (not sure about the name though), which, when set to true, will make so that the observable bound to value or values is updated upon the slide event instead of change.

I can split this into 2 distinct pull requests, if you are interested in only one of those 2 changes.

gvas commented 10 years ago

Hey @manuel-guilbault,

thanks for the pull request! It seems like a useful change. One thing that has confused me is that as far as I can tell the 2 new specs do the same thing. Additionally, setting the values option is already covered by the first spec in the slider.spec.js file (the one with the 'should handle each option of the widget'). Am I missing something here?

manuel-guilbault commented 10 years ago

Hi @gvas

You're welcome! I needed those features, so I thought I could share :)

You're right, the 2 specs do the same thing. I initially added the 2nd one to test the realtime option, but I didn't find a way to simulate a slide event on jquery's slider widget, and I forgot to remove the duplicate. Do you know a way to do this? If there is no way to automate this test, I'll remove the second spec.

As for the values option, unless I misunderstood the existing specs, it is partially covered by the 'should handle each option of the widget' spec: the existing spec makes sure that the widget's values option is changed when the binded observable changes, but it does not test the other way around. The spec I added makes sure that the binded observable is changed when the widget's values option changes.

gvas commented 10 years ago

The jQuery UI team uses the jquery-simulate to simulate mouse events. Here is one of their tests as an example. That library should be added to the knockout-jqueryui project under the lib/jquery-simulate directory.

You are right, your new specs test the widget->model direction, which aren't covered yet.

Thanks!

manuel-guilbault commented 10 years ago

OK I'll fix the specs and send a new pull request.

Thanks!

manuel-guilbault commented 10 years ago

Done!

gvas commented 10 years ago

Perfect. Thanks again!