Remove usage of setNativeProps #10934
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
bot
requested review from
stitesExpensify
and removed request for
a team
| @@ -67,6 +63,7 @@ class InvertedFlatList extends React.Component { | |||
| ref={el => this.list = el} | |||
| shouldMeasureItems | |||
| contentContainerStyle={StyleSheet.compose(this.props.contentContainerStyle, styles.justifyContentEnd)} | |||
| style={styles.translate0InvertY} | |||
There was a problem hiding this comment.
Context - we have to remove usage of setNativeProps from app as a prereq to enabling Fabric.
@marcaaron do you remember if there's any downside to pass this style as a prop vs using setNativeProps?
There was a problem hiding this comment.
Good Change if this works. the setnativeProps were causing issues.
There was a problem hiding this comment.
setnativeProps were causing issues
out of curiousity, do you remember what kind of issues?
There was a problem hiding this comment.
Update: This is fixed - #10934 (comment)
Bug - AddressSearch when used with Form is broken. (https://expensify.slack.com/archives/C01GTK53T8Q/p1663164808524219)
- Go to Settings -> Payments -> Add payment method -> Add debit card
- Search for any billing address
- Click a suggested address
Expected: Street address, City, State and Zipcode are autofilled.
Actual: Nothing happens
This is because Form was designed to be uncontrolled. I fixed this by automatically passing a value prop to all Form inputs. Todo:
|
|
asked for @parasharrajat's review to get the wheels rolling |
| // eslint-disable-next-line no-param-reassign | ||
| node.setNativeProps = this.setNativeProps; | ||
| }} | ||
| ref={this.props.innerRef} |
There was a problem hiding this comment.
did i do this right?
There was a problem hiding this comment.
Form passes a function to register this ref, so we should do something similar to this, no?
package.json
Outdated
| "@storybook/builder-webpack5": "^6.5.10", | ||
| "@storybook/manager-webpack5": "^6.5.10", |
There was a problem hiding this comment.
Why is this change in this PR?
There was a problem hiding this comment.
it's so that we can test on storybook.
storybook is currently broken on main.
I created a seperate PR for fixing storybook - #10933
This diff will go away once it's merged
There was a problem hiding this comment.
Merging main should get rid of this diff since the storybook PR is merged
| @@ -67,6 +63,7 @@ class InvertedFlatList extends React.Component { | |||
| ref={el => this.list = el} | |||
| shouldMeasureItems | |||
| contentContainerStyle={StyleSheet.compose(this.props.contentContainerStyle, styles.justifyContentEnd)} | |||
| style={styles.translate0InvertY} | |||
There was a problem hiding this comment.
Good Change if this works. the setnativeProps were causing issues.
| @@ -2671,6 +2671,10 @@ const styles = { | |||
| fontFamily: fontFamily.GTA, | |||
| marginLeft: 6, | |||
| }, | |||
|
|
|||
| translate0InvertY: { | |||
| transform: 'translate3d(0,0,0) scaleY(-1)', | |||
There was a problem hiding this comment.
This should be causing issues on Native if used. Even if this is used for the web, we can't put it into the global stylesheet. Use proper format for the transformation in react-native. In the future, someone can use it or can be confused by it.
There was a problem hiding this comment.
Oh yeahh, didn't think about it.
I'll see where it could be moved
| value={this.props.value || this.pickerValue} | ||
| value={this.props.value || this.props.defaultValue} |
There was a problem hiding this comment.
The concept of defaultValue is unnecessary when we are using state-bound value prop.
There was a problem hiding this comment.
Agree! I'll explain why I didn't remove it in this PR.
I think all Form components are state based / value prop driven. But Form.js passes defaultValue to all it's children anyway because it was designed to be uncontrolled.
I reckon we should do the cleanup in a follow-up PR.
Line 154 in 97ebb65
I wanted to avoid adding more tests or break anything basically
There was a problem hiding this comment.
For example, StatePicker uses defaultValue.
We'd be introducing unrelated diffs and tests.
| @@ -56,17 +37,9 @@ class BasePicker extends React.Component { | |||
| pickerProps={{ | |||
| onFocus: this.props.onOpen, | |||
| onBlur: this.executeOnCloseAndOnBlur, | |||
| ref: this.props.innerRef, | |||
There was a problem hiding this comment.
I don't know. Must be a mistake :))
There was a problem hiding this comment.
I think we should remove this
|
|
||
| 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, this.props.inputID); | ||
| return; | ||
| } | ||
|
|
||
| this.props.onInputChange(value, index); |
There was a problem hiding this comment.
How is this change related to this PR?
There was a problem hiding this comment.
Great question! @parasharrajat
Anytime a Form input's value is changed, onInputChange() is called.
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.
And this "key" is supposed to be the formID used by the Form input.
So our expected function call looks something like this - onInputChange('Portugal', 'countryPicker')
Prerequisite: Picker is used in a Form.
Now RNPickerSelect comes into picture.
It passes the picker item's index as the second parameter. (source)
So this is what actually gets called - onInputChange('Portugal', 156). (where 156 is the index of Portugal in a list)
Our Form based picker now will try to change the value of an input whose formID is 156.
And bam 💥 our Form based picker doesn't work anymore.
I'm just fixing this by passing the formID as the second param, because we don't care aobut the index lol
There was a problem hiding this comment.
This wasn't a problem before because previously, Form set the value using ref.setNativeProps.
We're getting rid of that, so I had to fix Picker in this PR only.
Line 166 in 1c78eb8
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
|
@parasharrajat please let me know once you are done reviewing this one. |
|
Sure, Can I get assigned to the issue as well? |
|
@parasharrajat hmm I see that this PR is only a part of the linked issue. Do you intend to work on the full migration to Fabric? |
| } | ||
|
|
||
| return React.cloneElement(child, { | ||
| ref: node => this.inputRefs[inputID] = node, | ||
| defaultValue, | ||
| value: this.state.inputValues[inputID], |
There was a problem hiding this comment.
So this means that inputs are no longer uncontrolled?
There was a problem hiding this comment.
@luacmartins yes, that's right
| @@ -56,17 +37,9 @@ class BasePicker extends React.Component { | |||
| pickerProps={{ | |||
| onFocus: this.props.onOpen, | |||
| onBlur: this.executeOnCloseAndOnBlur, | |||
| ref: this.props.innerRef, | |||
There was a problem hiding this comment.
I think we should remove this
| // eslint-disable-next-line no-param-reassign | ||
| node.setNativeProps = this.setNativeProps; | ||
| }} | ||
| ref={this.props.innerRef} |
There was a problem hiding this comment.
Form passes a function to register this ref, so we should do something similar to this, no?
| this.setState({ | ||
| isCommentEmpty: !!newComment.match(/^(\s|`)*$/), | ||
| value: newComment, |
There was a problem hiding this comment.
We should add value to state
| @@ -97,7 +97,6 @@ class ReportActionItemMessageEdit extends React.Component { | |||
| * @param {String} newDraft | |||
| */ | |||
| updateDraft(newDraft) { | |||
| this.textInput.setNativeProps({text: newDraft}); | |||
There was a problem hiding this comment.
Let's make sure this issue doesn't come back
There was a problem hiding this comment.
yep, I have already added tests for it
| @@ -291,7 +291,6 @@ class ProfilePage extends Component { | |||
| items={timezones} | |||
| isDisabled={this.state.isAutomaticTimezone} | |||
| defaultValue={this.state.selectedTimezone} | |||
| value={this.state.selectedTimezone} | |||
There was a problem hiding this comment.
Can we remove other usages of this.state.selectedTimezone in this file?
There was a problem hiding this comment.
Can't because it's controlled.
I now removed defaultValue, and passed value.
| @@ -2671,6 +2671,10 @@ const styles = { | |||
| fontFamily: fontFamily.GTA, | |||
| marginLeft: 6, | |||
| }, | |||
|
|
|||
| translate0InvertY: { | |||
| transform: 'translate3d(0,0,0) scaleY(-1)', | |||
There was a problem hiding this comment.
I think this is the correct format?
| transform: 'translate3d(0,0,0) scaleY(-1)', | |
| transform: [{translateX: 0}, {translateY: 0}, {scaleY: -1}], |
There was a problem hiding this comment.
Will have to verify. I'll breakup all this into multiple PRs.
@luacmartins I agree. |
Why?
setNativePropswill not be supported in the post-Fabric world (docs). Removing it's usage is a prerequisite to enable Fabric in #8503Details
For SignInPageForm, TextInputLabel/index and TextInput/index
Usage of
setNativePropsis migrated tosetAttributeFor ReportActionCompose, Form, OptionsSelector and ReportSettingsPage
Usage of
setNativePropsis migrated to stateFor InvertedFlatList
Usage of
setNativePropsis migrated by passing styles.translate0InvertYas a propFor Picker and ReportActionItemMessageEdit
Usage of
setNativePropsis just removed. No migration was needed.RoomNameInput was modified to support controlled inputs using
valueprop.Fixed Issues
$ #8503
(prerequisite step)
Tests
Lotsss of tests here. Brace yourself!
Sign In page
New group page / OptionsSelector
Message Edit
And
On web and desktop
Picker
npm run storybookand test picker functionalities. Make sure that we can edit the input and all values are updated accordinly.Form component
npm run storybookand test Form. Make sure that we can edit the input and all values are updated accordingly.Address search
Pages using Form.js
Chat / InvertedFlatList
On Safari and Firefox,
Composer
This step is not applicable for QA - Room name input
Prerequisite - comment out the validations in ReportSettingsPage
Screen.Recording.2022-09-10.at.6.31.51.PM.mov
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Sign In page
New group page / OptionsSelector
Message Edit
And
On web and desktop
Picker
Form component
Pages using Form.js
Chat / InvertedFlatList
On Safari and Firefox,
Composer
Screenshots
Web
Mobile Web
Desktop
iOS
Android