-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Form - Migrate setNativeProps to state. #11039
Changes from all commits
1e1765a
58505b4
50373ab
a7f4f88
3fb74b2
d3e630b
68c1389
c5ce2d3
c8f4299
55bcc8f
951be8f
44b8a95
2cf3288
92dc75d
f505376
f19213c
addf6fc
c7bef85
3b01ab9
150f736
28f5db3
426e24f
b1a6d98
d8e66e8
41d0ebd
24bd378
ff83af2
ddc4050
1273efc
aab173b
c72e7ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -29,6 +29,9 @@ const propTypes = { | |||||||||||||
|
||||||||||||||
/** Saves a draft of the input value when used in a form */ | ||||||||||||||
shouldSaveDraft: PropTypes.bool, | ||||||||||||||
|
||||||||||||||
/** A callback method that is called when the value changes and it receives the selected value as an argument */ | ||||||||||||||
onInputChange: PropTypes.func.isRequired, | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
const defaultProps = { | ||||||||||||||
|
@@ -47,6 +50,23 @@ class Picker extends PureComponent { | |||||||||||||
this.state = { | ||||||||||||||
isOpen: false, | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
this.onInputChange = this.onInputChange.bind(this); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Forms use inputID to set values. But Picker passes an index as the second parameter to onInputChange | ||||||||||||||
* We are overriding this behavior to make Picker work with Form | ||||||||||||||
* @param {String} value | ||||||||||||||
* @param {Number} index | ||||||||||||||
*/ | ||||||||||||||
onInputChange(value, index) { | ||||||||||||||
if (this.props.inputID) { | ||||||||||||||
this.props.onInputChange(value); | ||||||||||||||
return; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
this.props.onInputChange(value, index); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question from @parasharrajat - How is this change related to removing Rushat: Anytime a Form input's value is changed, If you'll look at Form.js Lines 161 to 166 in 97ebb65
It keeps track of input values based on the key provided to it. So our expected function call looks something like this - Prerequisite: Picker is used in a Form. Now It passes the picker item's index as the second parameter. (source) So this is what actually gets called - Our Form based picker now will try to change the value of an input whose formID is 156. I'm just fixing this by passing the formID as the second param, because we don't care aobut the index lol This wasn't a problem before because Form previously set the value using ref.setNativeProps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explanation makes sense, but I'm not sure that this is necessary. From the Form docs, the intent of the second param is to:
If you look at Form, we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rushatgabhane what are your thoughts about my comment above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just started reading this, will get back soon There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thank you, I understand the use of 2nd parameter better now. |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
render() { | ||||||||||||||
|
@@ -72,6 +92,7 @@ class Picker extends PureComponent { | |||||||||||||
value={this.props.value} | ||||||||||||||
// eslint-disable-next-line react/jsx-props-no-spreading | ||||||||||||||
{...pickerProps} | ||||||||||||||
onInputChange={this.onInputChange} | ||||||||||||||
/> | ||||||||||||||
</View> | ||||||||||||||
<InlineErrorText styles={[styles.mh3]}> | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,8 @@ class ProfilePage extends Component { | |
this.getLogins = this.getLogins.bind(this); | ||
this.validate = this.validate.bind(this); | ||
this.updatePersonalDetails = this.updatePersonalDetails.bind(this); | ||
this.setPronouns = this.setPronouns.bind(this); | ||
this.setAutomaticTimezone = this.setAutomaticTimezone.bind(this); | ||
} | ||
|
||
componentDidUpdate(prevProps) { | ||
|
@@ -96,6 +98,36 @@ class ProfilePage extends Component { | |
this.setState(stateToUpdate); | ||
} | ||
|
||
/** | ||
* @param {String} pronouns | ||
*/ | ||
setPronouns(pronouns) { | ||
const hasSelfSelectedPronouns = pronouns === CONST.PRONOUNS.SELF_SELECT; | ||
this.pronouns = hasSelfSelectedPronouns ? '' : pronouns; | ||
|
||
if (this.state.hasSelfSelectedPronouns === hasSelfSelectedPronouns) { | ||
return; | ||
} | ||
|
||
this.setState({hasSelfSelectedPronouns}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: This function is doing too many things. |
||
} | ||
|
||
/** | ||
* Update the timezone picker's value to guessed timezone | ||
* @param {Boolean} isAutomaticTimezone | ||
*/ | ||
setAutomaticTimezone(isAutomaticTimezone) { | ||
if (!isAutomaticTimezone) { | ||
this.setState({isAutomaticTimezone}); | ||
return; | ||
} | ||
|
||
this.setState({ | ||
selectedTimezone: moment.tz.guess(), | ||
isAutomaticTimezone, | ||
}); | ||
} | ||
|
||
/** | ||
* Get the most validated login of each type | ||
* | ||
|
@@ -166,14 +198,6 @@ class ProfilePage extends Component { | |
[values.firstName, values.lastName, values.pronouns], | ||
); | ||
|
||
const hasSelfSelectedPronouns = values.pronouns === CONST.PRONOUNS.SELF_SELECT; | ||
this.pronouns = hasSelfSelectedPronouns ? '' : values.pronouns; | ||
this.setState({ | ||
hasSelfSelectedPronouns, | ||
isAutomaticTimezone: values.isAutomaticTimezone, | ||
selectedTimezone: values.isAutomaticTimezone ? moment.tz.guess() : values.timezone, | ||
}); | ||
|
||
if (hasFirstNameError) { | ||
errors.firstName = Localize.translateLocal('personalDetails.error.characterLimit', {limit: CONST.FORM_CHARACTER_LIMIT}); | ||
} | ||
|
@@ -262,6 +286,7 @@ class ProfilePage extends Component { | |
label: this.props.translate('profilePage.selectYourPronouns'), | ||
}} | ||
defaultValue={pronounsPickerValue} | ||
onValueChange={this.setPronouns} | ||
/> | ||
{this.state.hasSelfSelectedPronouns && ( | ||
<View style={styles.mt2}> | ||
|
@@ -291,14 +316,15 @@ class ProfilePage extends Component { | |
label={this.props.translate('profilePage.timezone')} | ||
items={timezones} | ||
isDisabled={this.state.isAutomaticTimezone} | ||
defaultValue={this.state.selectedTimezone} | ||
value={this.state.selectedTimezone} | ||
onValueChange={selectedTimezone => this.setState({selectedTimezone})} | ||
/> | ||
</View> | ||
<CheckboxWithLabel | ||
inputID="isAutomaticTimezone" | ||
label={this.props.translate('profilePage.setMyTimezoneAutomatically')} | ||
defaultValue={this.state.isAutomaticTimezone} | ||
onValueChange={this.setAutomaticTimezone} | ||
/> | ||
</Form> | ||
</ScreenWrapper> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just bumped into this code and noticed that we are mutating the state during rendering. I haven't seen any bug caused by this, but, as far as I know, a base assumption when working in React that the state won't mutate like this, otherwise, basic checks equality checks by reference become not reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any strong reason to mutate the state in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right that this shouldn't be done. I think the state will change, but we won't re-render. AFAIK we should always be using
setState
cc @rushatgabhaneThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this can be a technique to exactly do that, avoid re-rendering. In some cases it makes sense to avoid re-rendering.. but there are other ways of achieving this without having to mutate the state. There was a similar conversation here about a proposal suggesting to do this: #11086 (comment)
Summary:
useRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. I was aware that it isn't recommended to mutate state but chose to mutate because -
App/src/components/Form.js
Lines 166 to 167 in c72e7ac
When
value
is passed as a prop, the parent component is expected to maintain (init and update) the state and cause a re-render.If we had used
setState
, it'll cause an unnecessary re-render.The form component also needs the correct
inputValues
to call the validate function.App/src/components/Form.js
Line 176 in c72e7ac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are times when we need to trigger a re-render. eg: uncontrolled input changes.
setState
is called forinputValues
App/src/components/Form.js
Lines 178 to 181 in c72e7ac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. But why maintain another duplicate variable that we'll need to always keep in sync with
inputValues
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really gone in depth in this code, and I don't really plan to at the moment because of priorities, but even in the worse case of having duplicate variables, I still think it is better that doing something that is not expected in the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll raise a PR to clean this up (hopefully by end of this week)
This way I can dive deep and see if it causes any bugs.
note to self: deep clone everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks guys!