loopj / jquery-simple-slider

Unobtrusive numerical slider plugin for jQuery
194 stars 113 forks source link

Make animate optional via data-slider-* attribute #24

Closed maartenvg closed 11 years ago

maartenvg commented 11 years ago

I was missing a data-slider-animate to toggle animation, so I created it for you.

It includes edits for all three formats, coffeescript, js and min.js

loopj commented 11 years ago

Hey Maarten, thanks for the pull request.

Could you ensure that this setting defaults to true for backwards compatibility? If so, I'd be happy to merge.

Best

maartenvg commented 11 years ago

No problem!

maartenvg commented 11 years ago

@loopj I added a commit that checks for the attribute.

maartenvg commented 11 years ago

I fetch/rebased the changes of todays commits, so now it should merge properly again

loopj commented 11 years ago

Thanks @maartenvg. I've noticed that while you've made a new setting, nothing actually checks the value of the setting, so your changes don't do anything.

loopj commented 11 years ago

Also, might i suggest an alternative approach for checking for "true/false" values from a data tag:

settings.animate = if $el.data("slider-animate")? then $el.data("slider-animate") else true

The ? operator in coffeescript is great for checking for a non-null value.

maartenvg commented 11 years ago

settings.animate is an existing setting that is used throughout the script to toggle animations. You implemented that yourself ;)

The only thing I did, was to make that setting editable via data-slider-animate, instead of only being able to set it via constructor initialization (losing all the pretty unobtrusive fun).

Thanks for the ? tip, I'll use that instead. I'm still a bit rusty on coffee-script.

loopj commented 11 years ago

I totally forgot I already had settings.animate!

loopj commented 11 years ago

Thanks @maartenvg