Fix/date time picker to controlled #989
Conversation
There was a problem hiding this comment.
Few comments on this PR
- The PR doesn't change the component to be a controlled component cause it still keep the value in an internal state instead of getting it from the user props.
- This is a breaking change to the component and we can't push it directly to master.
It means the user must pass the value prop, otherwise it won't react to changes.
It should be part of v6. - We need to make sure One App users migrate accordingly before we merge this.
| static getDerivedStateFromProps(nextProps, prevState) { | ||
| if (nextProps.value !== prevState.prevValue) { | ||
| return { | ||
| prevValue: prevState.value, | ||
| value: nextProps.value | ||
| }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
There's no need to keep the value in state if the component is controlled.
There was a problem hiding this comment.
I need to keep an internal state for the onChange
| @@ -133,14 +141,14 @@ class DateTimePicker extends BaseComponent { | |||
| // since handleChange() is not called on iOS when there is no actual change | |||
| this.chosenDate = new Date(); | |||
| } | |||
|
|
|||
|
|
|||
| _.invoke(this.props, 'onChange', this.chosenDate); | |||
| this.setState({value: this.chosenDate}); | |||
| }); | |||
|
|
|||
| getStringValue = () => { | |||
| const {value} = this.state; | |||
There was a problem hiding this comment.
If the component is controlled, the value should derived directly from props and not the state.
There was a problem hiding this comment.
You're right, generally speaking, but when we want a controlled component and still need to keep an internal state that's the way to do it. See Carousel, DialogDismissibleView, PageControl, Picker, GradientSlider for example.
…DateTimePicker_to_controlled
|
@ethanshar I fixed the branching, should be only 1 file change. I don't see a breaking change here, the user doesn't have to pass 'value' prop. The change is made for when the user does pass 'value' prop and change it with screen state change. |
…hub.com:wix/react-native-ui-lib into fix/DateTimePicker_to_controlled # Conflicts: # src/components/dateTimePicker/index.js
Description
DateTimePicker - turn to a controlled component. Fix issue #729
Changelog
DateTimePicker - turn to a controlled component.