-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Rangeslider: Implement Classes Option #8116
Rangeslider: Implement Classes Option #8116
Conversation
add backcompat Fixes jquery-archivegh-7717
add classes Fixes jquery-archivegh-7717
assert classes Fixes jquery-archivegh-7717
assert classes fix Fixes jquery-archivegh-7717
@@ -58,15 +55,17 @@ | |||
_sliderFirst = _sliderWidgetFirst.slider, | |||
_sliderLast = _sliderWidgetLast.slider, | |||
firstHandle = _sliderWidgetFirst.handle, | |||
_sliders = $( "<div class='ui-rangeslider-sliders' />" ).appendTo( $el ); | |||
_sliders = $( "<div />" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just <div>
changes from review Fixes jquery-archivegh-7717
_inputFirst = $el.find( "input" ).first(), | ||
_inputLast = $el.find( "input" ).last(), | ||
_label = $el.find( "label" ).first(), | ||
var rangeslider = this.element, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets drop the extra var
here rangeslider
is only 1 char shorter then this.element
and gzip likes repeated strings :-)
Overall looking good just a couple smallish things. In addition to the comments already there.
|
alter backcompat test file to include core Fixes jquery-archivegh-7717
time for another round @arschmitz |
|
||
if ( this.options.mini !== undefined ) { | ||
this.element.toggleClass( "ui-mini", !!this.options.mini ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't mini
already handled by the backcompat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @cgack on irc there is a bigger issue here to be looked into
move mini to backcompat Fixes jquery-archivegh-7717
@arschmitz I moved ui-mini to backcompat but could not fully remove it to get the classes applied correctly for the demo to look correct. Lemme know if you see something i'm missing when you review again. |
The 2 remaining changes are minor you can go ahead make them rebase and land i don't need to look again. 👍 |
landed with b6e619e |
Fixes gh-7717