fix for #754 prevent ColorWrap to reset own state #791
Conversation
The issue is that ColorWrap HOC manages color state but at the same time accepts "color" prop which affects the state in getDerivedStateFromProps. Because getDerivedStateFromProps is called every render, it always replaces state without acutal checking if the color property was changed. In the demo https://codesandbox.io/s/custom-picker-new-react-ur2z4 mentioned in casesandberg#754 the component has initial value 'orange' - it will derive to state every render. I added a step to check if property was changed to prevent resetting internal state. But to be honest I suggest to change the API of the component to avoid confusion. In the example ```tsx <MyPicker color="orange" onChangeComplete={handleColorChange} /> ``` Color prop is always passed so it should never change it by its own. If the intention is to provide initial value for the component, I think we should add 'defaultValue' property to indicate that it will be used only once.
|
@casesandberg what is blocking us from finalising this? |
This component API design is fine - a component should accept both a controlled prop (in this case
This is a bit unrelated, but I agree. A @adamborowski what do you think about completely removing The only reason I can think of is if the P.S. Thanks for putting up a pull request for the issue 🙏🏼 . |
|
Actually I don't use this component but needed to help someone using it but after analyzing the code I think I understand the purpose of getDerivedStateFromProps. So, we have color prop which is used to control the value, onChange prop which is called all the time user drags the mouse, and onChangeComplete which is called when user finishes dragging and releases mouse. We have two ways how user might want use component in a controlled mode. First, when users want to control current position on the "slider" and update state even when user drags the mouse. To do that, color and onChange props can be used. Second, when users want the control only final state of the "slider" and update state only when user finishes dragging the mouse and releases the mouse. To do that, color and onChangeComplete props can be used. So for the second case - user wants to control the state only if user isn't dragging the mouse. So for the time user is dragging mouse - picker has to manage the state internally until mouse is released. This means that the second case isn't fully controlled mode - it's a mixed mode. This is why we need to manage derived state. And I think it makes sense. For example, when we have a combo box - we might want to control the value but we expect onChange callback to by called not during typing text but only after user finishes typing and selects his preferred option. |
|
After taking another look at Still — there is no clear reason as to why the |
|
The problem @hzhu is that I can't find |
I agree that this is a problem and I think the API design of this component is a bit unconventional. I agree that it should have a
This isn't what I'm suggesting — I'm not suggesting removing the internal state. My suggestion is to remove the static method deriving state from props. |
If you do so, you won't be able to use the component in a controlled mode at all. Picker's color prop, if provided, will be used only initially |
|
The Here's a fork demonstrating the removal of Fork: https://github.com/hzhu/react-color |
|
@casesandberg - Do you think you could take a look at this please? |
|
@hzhu can you provide a code snippets of how you imagine using that ColorWrap component? |
Thank you for this. This is the only way I was able to run the storybook script. Their original repo is buggy. |
The issue is that ColorWrap HOC manages color state but at the same time accepts "color" prop which affects the state in getDerivedStateFromProps.
Because getDerivedStateFromProps is called every render, it always replaces state without actual checking if the color property was changed.
In the demo https://codesandbox.io/s/custom-picker-new-react-ur2z4 mentioned in #754 the component has initial value 'orange' - but it will overwrite component internal state every render.
I added a step to check if property was changed to prevent resetting internal state.
But to be honest I suggest to change the API of the component to avoid confusion.
In the example
Color prop is always passed so it should never change it by its own.
If the intention is to provide initial value for the component, I think we should add 'defaultValue' property to indicate that it will be used only once.