ui.slider: modified , so that slider value can not be set more than max.... #1016
Conversation
|
We shouldn't be calculating the max on every move/change. We should calculate once, whenever Also, we already have a function to abstract out the getting of the max value. You shouldn't be adding a new function, you should be changing |
|
Please also take some time to read our style guide as there are many style violations in this code. |
|
So are you recommending - override |
|
No, calculate the max and store it in a different property on the plugin. Then have |
|
Will it be ok ? if |
|
I'd much rather you follow existing conventions and calculate it when it actually changes. |
|
@scottgonzalez submitted new code and ran |
| @@ -531,12 +533,29 @@ $.widget( "ui.slider", $.ui.mouse, { | |||
| return parseFloat( alignValue.toFixed(5) ); | |||
| }, | |||
|
|
|||
| _calculateNewMax: function() { | |||
| var newmax, | |||
| step = ( this.options.step > 0 ) ? this.options.step : 1; | |||
There was a problem hiding this comment.
We shouldn't be guarding against this. If the step is set to a negative number, then we can force the option to 1. But unless we have a bug report for this, I'd rather just skip it.
There was a problem hiding this comment.
i removed this check in new commit - 90ca5b7436d88bff98ce4c6d01b51236cbb88fe9
|
You still need to handle the case of |
| @@ -56,6 +56,44 @@ test( "max", function() { | |||
| ok(element.slider( "value" ) === options.max, "value method is contained by max" ); | |||
| element.slider( "destroy" ); | |||
|
|
|||
| element = $( "<div></div>" ); | |||
|
|
|||
| options = { | |||
There was a problem hiding this comment.
Don't redefine options. Just update the one option you care about so that it's clear what you're testing.
|
Don't be discouraged by the large number of comments, it's mostly really small stuff. This is getting really close. Thanks for sticking with this. |
|
sure :) i will implement the changes |
|
@scottgonzalez i updated the code . hope this time i get it right |
|
Rebased commits into 1 commit. updated the email address. |
|
@dekajp I'm assuming this is ready for us to take a look at again correct? |
|
@tjvantoll yes |
|
👍 |
| @@ -531,12 +534,22 @@ $.widget( "ui.slider", $.ui.mouse, { | |||
| return parseFloat( alignValue.toFixed(5) ); | |||
| }, | |||
|
|
|||
| _calculateNewMax: function() { | |||
There was a problem hiding this comment.
this can be done with math instead of a loop (untested):
using division:
var steps = Math.floor( ( this.options.max - this._valueMin() ) / this.options.step );
this.max = this._valueMin() + ( steps * this.options.step );
using remainders:
var remainder = ( this.options.max - this._valueMin() ) % this.options.step;
this.max = this.options.max - remainder;
|
@dekajp can you update this PR to use math instead of a loop? |
|
@mikesherov - removed loop |
|
@mikesherov @tjvantoll can you take a look at this again? Should be good to land. |
…Fixed #9376 - Slider: Changed _max to max
…i.slider: On mouse move slider sets value more than max
ada0337 to
8932df7
Compare
|
Rebased /cc @mikesherov @tjvantoll @jzaefferer |
|
This fix looks good to me and I verified it works. All of @scottgonzalez's and @mikesherov's comment were addressed, so I'm going to land this. |
...Fixed #9376 - ui.slider: On mouse move slider sets value more than max