tleunen / react-number-editor

Custom number editor (text field) react component
MIT License
70 stars 15 forks source link

Make it controllable? #5

Closed gre closed 9 years ago

gre commented 9 years ago

Hi @tleunen,

I might be interested by replacing my input[type=number] usage by your component in diaporama-maker, however my concern is that I would like to use it in a controlled way (not uncontrolled, where the state is internal and the onValueChange is a side effect).

Do you think if it is possible?

Basically it would mean not having the state value internally stored in the component but moving that state up in the tree and passing it in a value props instead. (The lib user control it, can do more validation, can easily synchronise that value from other inputs (e.g for some reason you want to change the value of react-number-editor)).

Then you can use uncontrollable to make a controllable component also uncontrollable. It reconciliates both the defaultValue and value usages (so that allows to make a component either controllable or uncontrollable like React input and select components)

Here is an example of a component using uncontrollable: http://greweb.me/bezier-easing-editor/example/ What is great with that is, in your implementation, you can get rid of internal state and uncontrollable guarantees that you will always get the *value in props and you can just call this.props.on*Change.

tleunen commented 9 years ago

Why don't you set the initialValue when you want to update the value of an input from the the parent component?

gre commented 9 years ago

because if you want to "control" the value, which means changing it afterwards, you can't do it with just an initialValue prop. (like for Controlled Components inputs)

Uncontrolled inputs are ok while you don't have the value of your input depending of something else, which can happens in different use-cases.

Simple use-case I have: out (there are 2 ways to edit the duration, either editing the input value, or resizing it on the timeline)

For this use-case it seems mandatory to have such a system. More generally I tends to prefer to move states at higher level of my apps so the whole application renders consistently with that state and most components are stateless. If you only store the state value at higher levels it is much easier to access (and you don't have to access it via refs or duplicating the states). I guess this way is one of general React best practice.

tleunen commented 9 years ago

But in this case, will we lose the feature of quickly increasing/decreasing the value by using special keys?

I understand the usecase, but I'm not sure what's the best way to do it and keep the features.

gre commented 9 years ago

It seems in your code that calling onValueChange occurs exactly when you also setState a new value.

https://github.com/tleunen/react-number-editor/blob/master/index.js#L71-L76

So I guess it wouldn't change much the behavior of your component (or did I miss something?). That is just about moving from a state value to a prop value?

To me what you might just need to do is :

And if you want to benefit of uncontrollable (that provides either controllable / uncontrollable features)

gre commented 9 years ago

(just in case it helps, here is an example I've recently wrote that also uses uncontrollable http://greweb.me/multi-slider/ – code https://github.com/gre/multi-slider/ )

tleunen commented 9 years ago

Hmm.. I don't really want to use another library to make it work (I mean uncontrollable). As you said, it seems I just need to remove the value from the state, and let the parent component handle the value. Anyway, it already does that with the onValueChange callback.

But I still don't understand something, because even if you want to set a specific value into the component. You could use initialValue. This will set the component to the value you want. Maybe I miss something here, I would need to test but it should work as expected imo.

gre commented 9 years ago

https://github.com/tleunen/react-number-editor/blob/master/index.js#L45-L47 ^ These are the only place where initialValue is used. And React call getInitialState() only once: at instanciation time, so (unless you force a new instance) initialValue is only used for the initial value, not for changing the value afterwards.

As you prefer, you could re-create what uncontrollable is doing if you want to both have "Controlled" and "Uncontrolled" usage.

tleunen commented 9 years ago

You're right. I should add some more logic in componentWillReceiveProps to use the new initialValue. But then I'll change it to just value and keep some logic inside the component and let the parent component manage it as well.

I don't have much time for now so I'm not sure when I'll be able to convert it, I'll try next weekend.

tleunen commented 9 years ago

Hey @gre, could you please check what I did in the controllable branch?

I removed the internal value so it's the responibility of the parent component to provide the value (and updated value). (I haven't updated the readme yet. The only main difference is really that you have to provide a value)

tleunen commented 9 years ago

any comment @gre ?