Slider: Setting values or range slider to max, doesn't lock slider #1502
Conversation
When both values are set to the maximum change them in descending order Fixes #9046
| valsLength = 0; | ||
| rangeValues = [ this.values( 0 ), this.values( 1 ), this._valueMax() ]; | ||
|
|
||
| if ( this.options.range === true && $.unique( rangeValues ).length === 1 ) { |
There was a problem hiding this comment.
Is this check actually necessary? Might be enough to just always do the reverse loop. I tested that locally with the new visual test page and the unit tests, works for both.
There was a problem hiding this comment.
Yes, I've also tested that, it works well. I thought that setting values with reverse loop to the minimum will lock slider, but didn't check it.
|
We generally avoid adding more visual test pages, whenever its possible to cover the same behaviour with unit tests instead. In this case, we already have one test that deals with overlapping handles - can you look into extending that test? |
|
Yes, after writing unit test, visual one will be unuseful I think it should be deleted |
|
Alright. So you could update the unit test first, once that passes with the patch in place and fails without, refactor the patch as discussed and delete the visual test. |
|
If everything is ok then I will remove a visual test |
| @@ -453,9 +460,7 @@ return $.widget( "ui.slider", $.ui.mouse, { | |||
| case "values": | |||
| this._animateOff = true; | |||
| this._refreshValue(); | |||
| for ( i = 0; i < valsLength; i += 1 ) { | |||
There was a problem hiding this comment.
Why not just modify this one line?
There was a problem hiding this comment.
👍
Now that the other changes are gone, the separate method isn't needed anymore. The updated for-loop deserves a line comment explaining the reverse iteration.
|
For me it seems that there is still an issue, please have a look at this fiddle. I encountered the problem with a slider which has two handle, while setting its values to the maximum. It happens when both values are set at the same time, or when I set the value one by one with the handle [0] first. And strangely, if you set the handle [1] first, then it works perfectly fine. |
|
@eugsa It looks like the problem is fixed for standard multi-thumb sliders, but not for range sliders. Would you mind filing an issue at https://bugs.jqueryui.com/newticket and include a link to your test case? Thanks! |
When both values are set to the maximum change them in descending order
Fixes #9046