search

claus / react-dat-gui

React dat.GUI
MIT License
292 stars 56 forks source link

Number rounding in DatNumber #4

Closed rohan-deshpande closed 5 years ago

rohan-deshpande commented 7 years ago

Yeah, I've noticed this too now that I've started to use this and need a step of 0.01. I'm guessing this is due to how applyConstraints is working?

rohan-deshpande commented 7 years ago

Hey I've got a branch for this. The problem now though is because #3 isn't merged... we might get merge conflicts.

https://github.com/rohan-deshpande/react-dat-gui/tree/feature/support-floats

Checkout DatNumber and let me know what you think. Basically this is working how I think it should. The component will actually use the step you've given it now and also give you the correct number of decimal places you require as per the step's decimal places.

How close do you think you are to finishing the react-dependency branch? Because I'd be happy to revert #3 on my fork and cherry pick this branch's commits into a new one if you finish that feature. Then I can put mine back to the commit before the README changes, pull yours in and cherry pick the commits from my support-floats branch.

rohan-deshpande commented 7 years ago

Okay so, this is working well for the actual number input, but the slider isn't performing nicely with floats. I'll attempt to fix that before doing a PR.

Edit: Whoops sorry accidentally closed the issue, reopening.

claus commented 7 years ago

Yeah this whole number rounding problem is not easy. You can't possibly cover all min/max/step/initial value combinations. Like what about min: 0, max: 1000, step: 0.01, initial: -0.0009456? What do you do? It's not a trivial problem. Maybe look at how the original does it?

As for #3, i'm on it but likely need the weekend yet.

rohan-deshpande commented 7 years ago

If the min is 0 though it shouldn't allow an initial that's negative... it should force the value to conform to the props passed I reckon.

claus commented 7 years ago

Right, ok. So does it trigger an update when it clamps the value? What happens when i have two DatNumbers with different constraints that both operate on the same path? Etc. It's not easy :)

rohan-deshpande commented 7 years ago

It's currently working like that for me. You can have two that control the same value with different configs. The value will mutate according to the config you've passed to the control. I think that's expected.

The only problem for me right now is the slider. It needs to increment as a factor of the step I think. Right now, if I have a control which is like

<DatNumber 
  min={0} 
  max={100} 
  step={0.001} 
/>

The input field works perfectly. It'll ensure all values have 3 decimal places. If I use the mouse wheel inside the number input, it works exactly how I think the slider should work because the step is enforced.

The problem with the slider is that it doesn't care about the step so it lacks the level of control that the input has. It's actually kind of useless when you have a step that's a float.

rohan-deshpande commented 7 years ago

I've also just realised, after many hours of pain, that DatNumber is setting its values as strings! This was causing havoc for me. I'll fix that up as well.

rohan-deshpande commented 7 years ago

Hey long breaks between updates, I've fixed the typings of DatNumber, I've got a question though, was there any reason you triggered a state change onBlur for this component? This is causing issues for me, I currently see no issues when removing the onBlur handler from the component, just wondering if you did this for a specific use case?

omaralvarez commented 6 years ago

Is this going to be merged? I am having the same issue, and would love to see this merged.

Is the project dead (no commits since 2017)?

rohan-deshpande commented 6 years ago

Hi @omaralvarez you're commenting on an issue, not a PR, nothing to merge here. I think @claus and I need to fix up a couple of things in the current open PRs, I've got https://github.com/rohan-deshpande/react-dat-gui/tree/feature/support-floats which seems to fix the issue described here for me, I've been using it in my app for some time now with no problems. I will get a PR open for it.

rohan-deshpande commented 5 years ago

Resolved by #12