Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Floating step - different results with click and keyboard #179

Closed
viters opened this issue May 18, 2020 · 4 comments · Fixed by #187
Closed

Floating step - different results with click and keyboard #179

viters opened this issue May 18, 2020 · 4 comments · Fixed by #187

Comments

@viters
Copy link
Contributor

viters commented May 18, 2020

Description

When I set step to floating value, like 0.1 - using slider with mouse is okay and everything works as expected.

When I use keyboard to change slider value, there are floating point issues:

image

The issue is probably because of floating point operations at lines:
https://github.com/zillow/react-slider/blob/master/src/components/ReactSlider/ReactSlider.jsx#L730
https://github.com/zillow/react-slider/blob/master/src/components/ReactSlider/ReactSlider.jsx#L736

I could suggest solution like this:

[...]
const sanitizeFloatValue = (a) => Number.isInteger(a) ? a : parseFloat(a.toFixed(10));
[...]

const newValue = sanitizeFloatValue(oldValue + step);

image

@stonebk
If you think this solution is acceptable, I will make PR.

CodeSandbox

https://stackblitz.com/edit/react-oeujyh

@stonebk
Copy link
Contributor

stonebk commented May 18, 2020

@viters thanks for the detailed issue report!

I don't think I would add this kind of sanitization logic to the component itself because it is the result of using JavaScript floats and not really the fault of the slider logic.

Perhaps we can leverage our onBeforeChange lifecycle method as a place to run sanitization logic and return a different value.

@viters
Copy link
Contributor Author

viters commented May 18, 2020

I don't think I would add this kind of sanitization logic to the component itself because it is the result of using JavaScript floats and not really the fault of the slider logic.

@stonebk I think libraries do fight with JS sometimes to hide this fight from users.

There may be better solution, but I would not require users to handle this difference themselves. In other words, I think slider should produce the same results, regardless of interaction type (mouse, touch, keyboard). Moreover, I think it's a nice idea to imitate native controls - and native range slider does handle floats itself (https://stackblitz.com/edit/typescript-9gazv2?file=index.html).

Maybe there is a way to unify how newValue is calculated for mouse and keyboard?

@viters
Copy link
Contributor Author

viters commented May 19, 2020

One example of handling this situation is presented by rc-slider:
https://github.com/react-component/slider/blob/master/src/utils.ts
Especially ensureValuePrecision.

This is proper solution I think, setting the target precision based on step from props and ensuring the output value has the same precision. As as library user I would not expect receiving e.g. 0.1001 value when I have step of 0.1.

Sorry for open/close missclick.

@viters viters closed this as completed May 19, 2020
@viters viters reopened this May 19, 2020
@stonebk
Copy link
Contributor

stonebk commented Jun 15, 2020

Yeah, the more I think about it, it makes sense that we resolve the issue since we are the ones doing the addition/subtraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants