Max value calculation wrong due to cutoff when max value is not a multiple of step #759
Conversation
…alue does not fit the step size
|
please add a unit test so this particular behavior doesn't happen anymore! |
|
additionally, please make sure this passes our linter! |
|
I have added the test and checked the linter. However I have a problem with my test code. I did not find a useful way to get the outer right position for the slider right - i had to add 10px manually to the mouse-click. I do not know why this is the case but I figure you guys probably have a pointer for me :-). Then I will gladly fix the test and get rid of the "+10". For the mouse-clicking I copied the method from another test - this is where codeclimate complains now about code duplication. What is your suggested way of fixing this? |
test/specs/StepReachMaxValueSpec.js
Outdated
| min: 39, | ||
| max: 1310, | ||
| step: 5, | ||
| scale:"logarithmic", |
There was a problem hiding this comment.
test is solid! could you fix the spacing on this line?
test/specs/StepReachMaxValueSpec.js
Outdated
| it("Value should contain max value when slider is moved to outer right position", function() { | ||
|
|
||
| var sliderLeft = slider.sliderElem.offsetLeft; | ||
| var offsetY = slider.sliderElem.offsetTop; |
test/specs/StepReachMaxValueSpec.js
Outdated
| var offsetY = slider.sliderElem.offsetTop; | ||
| var offsetX = sliderLeft + slider.sliderElem.clientWidth+10; | ||
|
|
||
| var expectedValue = slider.options.max; |
as long as you feel the +10 value is roughly accurate, that is fine. maybe include a comment in the test about why you are using this value
Don't worry about this. I have been pondering completely removing the codeclimate check or just limiting it to the source directory. |
|
I cleaned up the test code, added the comment and fixed the intendation. |
|
@thaitzer just pulled down your branch to run the examples page and it works great! Thanks again for your contribution! |
|
@thaitzer btw, are there any existing issues from our issues list that this fix addresses? I just want to ensure those get listed in our changelog. |
|
@rovolution I did not check the issues before writing this mini-patch. So the answer is: not that I know of. While I am writing this: I never bothered to check if the same thing happens on the linear scale - I might be a good idea to do that. And: thank you guys, for creating and maintaining this cool project! |
if you find that this is also an issue, we would definitely appreciate another PR! |
gracefully handle max value when a step size is defined and the max v…alue does not fit the step size