Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Rangeslider: Implement Classes Option #8116

Closed

Conversation

cgack
Copy link
Contributor

@cgack cgack commented Apr 17, 2015

Fixes gh-7717

@@ -58,15 +55,17 @@
_sliderFirst = _sliderWidgetFirst.slider,
_sliderLast = _sliderWidgetLast.slider,
firstHandle = _sliderWidgetFirst.handle,
_sliders = $( "<div class='ui-rangeslider-sliders' />" ).appendTo( $el );
_sliders = $( "<div />" );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just <div>

_inputFirst = $el.find( "input" ).first(),
_inputLast = $el.find( "input" ).last(),
_label = $el.find( "label" ).first(),
var rangeslider = this.element,
Copy link
Contributor

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 :-)

@arschmitz
Copy link
Contributor

Overall looking good just a couple smallish things. In addition to the comments already there.

  • Add files your working on to jscs:good
  • In the demos the "mini" sliders have something funny going on there is an extra horizontal line below the track. This is not happening on 1.5-dev

alter backcompat test file to include core

Fixes jquery-archivegh-7717
@cgack
Copy link
Contributor Author

cgack commented May 30, 2015

time for another round @arschmitz


if ( this.options.mini !== undefined ) {
this.element.toggleClass( "ui-mini", !!this.options.mini );
}
Copy link
Contributor

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?

Copy link
Contributor

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

@cgack
Copy link
Contributor Author

cgack commented Jun 1, 2015

@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.

@arschmitz
Copy link
Contributor

The 2 remaining changes are minor you can go ahead make them rebase and land i don't need to look again. 👍

@arschmitz arschmitz assigned cgack and unassigned arschmitz Jun 4, 2015
cgack added a commit that referenced this pull request Jun 4, 2015
@cgack
Copy link
Contributor Author

cgack commented Jun 4, 2015

landed with b6e619e

@cgack cgack closed this Jun 4, 2015
arschmitz pushed a commit to arschmitz/jquery-mobile that referenced this pull request Jun 9, 2015
cgack added a commit that referenced this pull request Jun 9, 2015
arschmitz pushed a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants