Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Form - Migrate setNativeProps to state. #11039

Merged
merged 31 commits into from
Oct 7, 2022
Merged

Form - Migrate setNativeProps to state. #11039

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1e1765a
make form controlled by moving inputValues to state
rushatgabhane Sep 16, 2022
58505b4
Make Picker controlled only and remove setNativeProps
rushatgabhane Sep 16, 2022
50373ab
make picker work with Form
rushatgabhane Sep 16, 2022
a7f4f88
remove defaultValue prop from Picker
rushatgabhane Sep 16, 2022
3fb74b2
fix eslint error
rushatgabhane Sep 16, 2022
d3e630b
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Sep 18, 2022
68c1389
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Oct 4, 2022
c5ce2d3
remove default value
rushatgabhane Oct 4, 2022
c8f4299
Revert "remove default value"
rushatgabhane Oct 4, 2022
55bcc8f
use default value for pickers
rushatgabhane Oct 4, 2022
951be8f
use props.value in Form
rushatgabhane Oct 4, 2022
44b8a95
remove console log
rushatgabhane Oct 4, 2022
2cf3288
allow form consumer to get the value changes
rushatgabhane Oct 5, 2022
92dc75d
fix form stories
rushatgabhane Oct 5, 2022
f505376
add onInputChange to picker stories
rushatgabhane Oct 5, 2022
f19213c
eslint
rushatgabhane Oct 5, 2022
addf6fc
eslint
rushatgabhane Oct 5, 2022
c7bef85
rename custom callback onInputChange to onValueChange
rushatgabhane Oct 5, 2022
3b01ab9
cleanup hack
rushatgabhane Oct 5, 2022
150f736
fix lint errors
rushatgabhane Oct 5, 2022
28f5db3
Merge branch 'main' into rm-setNativeProps-form
rushatgabhane Oct 5, 2022
426e24f
refactor: don't pass inputID
rushatgabhane Oct 5, 2022
b1a6d98
refactor: move callback to named function
rushatgabhane Oct 5, 2022
d8e66e8
fix: eslint errors
rushatgabhane Oct 5, 2022
41d0ebd
docs: remove defaultValue, and add value
rushatgabhane Oct 5, 2022
24bd378
docs: add all props available for form inputs
rushatgabhane Oct 5, 2022
ff83af2
fix typo
rushatgabhane Oct 6, 2022
ddc4050
fix: pickers on company info page
rushatgabhane Oct 6, 2022
1273efc
Merge branch 'rm-setNativeProps-form' of github.com:rushatgabhane/Exp…
rushatgabhane Oct 6, 2022
aab173b
fix: picker closing before clicking done on ios
rushatgabhane Oct 6, 2022
c72e7ac
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/components/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ class Form extends React.Component {

this.state = {
errors: {},
inputValues: {},
};

this.inputRefs = {};
this.inputValues = {};
this.touchedInputs = {};

this.setTouchedInput = this.setTouchedInput.bind(this);
Expand All @@ -87,12 +87,12 @@ class Form extends React.Component {
));

// Validate form and return early if any errors are found
if (!_.isEmpty(this.validate(this.inputValues))) {
if (!_.isEmpty(this.validate(this.state.inputValues))) {
return;
}

// Call submit handler
this.props.onSubmit(this.inputValues);
this.props.onSubmit(this.state.inputValues);
}

/**
Expand Down Expand Up @@ -145,30 +145,31 @@ class Form extends React.Component {
const defaultValue = this.props.draftValues[inputID] || child.props.defaultValue;

// We want to initialize the input value if it's undefined
if (_.isUndefined(this.inputValues[inputID])) {
this.inputValues[inputID] = defaultValue;
if (_.isUndefined(this.state.inputValues[inputID])) {
this.state.inputValues[inputID] = defaultValue;
}

return React.cloneElement(child, {
ref: node => this.inputRefs[inputID] = node,
defaultValue,
value: this.state.inputValues[inputID],
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
errorText: this.state.errors[inputID] || '',
onBlur: () => {
this.setTouchedInput(inputID);
this.validate(this.inputValues);
this.validate(this.state.inputValues);
},
onInputChange: (value, key) => {
const inputKey = key || inputID;
this.inputValues[inputKey] = value;
const inputRef = this.inputRefs[inputKey];
this.setState(prevState => ({
inputValues: {
...prevState.inputValues,
[inputKey]: value,
},
}), () => this.validate(this.state.inputValues));

if (key && inputRef && _.isFunction(inputRef.setNativeProps)) {
inputRef.setNativeProps({value});
}
if (child.props.shouldSaveDraft) {
FormActions.setDraftValues(this.props.formID, {[inputKey]: value});
}
this.validate(this.inputValues);
},
});
});
Expand Down
33 changes: 5 additions & 28 deletions src/components/Picker/BasePicker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,7 @@ class BasePicker extends React.Component {
constructor(props) {
super(props);

this.pickerValue = this.props.defaultValue;

this.updateSelectedValueAndExecuteOnChange = this.updateSelectedValueAndExecuteOnChange.bind(this);
this.executeOnCloseAndOnBlur = this.executeOnCloseAndOnBlur.bind(this);
this.setNativeProps = this.setNativeProps.bind(this);
}

/**
* This method mimicks RN's setNativeProps method. It's exposed to Picker's ref and can be used by other components
* to directly manipulate Picker's value when Picker is used as an uncontrolled input.
*
* @param {*} value
*/
setNativeProps({value}) {
this.pickerValue = value;
}

updateSelectedValueAndExecuteOnChange(value) {
this.props.onInputChange(value);
this.pickerValue = value;
}

executeOnCloseAndOnBlur() {
Expand All @@ -42,12 +23,12 @@ class BasePicker extends React.Component {
const hasError = !_.isEmpty(this.props.errorText);
return (
<RNPickerSelect
onValueChange={this.updateSelectedValueAndExecuteOnChange}
onValueChange={this.props.onInputChange}
items={this.props.items}
style={this.props.size === 'normal' ? basePickerStyles(this.props.disabled, hasError, this.props.focused) : styles.pickerSmall}
useNativeAndroidPickerStyle={false}
placeholder={this.props.placeholder}
value={this.props.value || this.pickerValue}
value={this.props.value || this.props.defaultValue}
Icon={() => this.props.icon(this.props.size)}
disabled={this.props.disabled}
fixAndroidTouchableBug
Expand All @@ -57,15 +38,11 @@ class BasePicker extends React.Component {
onFocus: this.props.onOpen,
onBlur: this.executeOnCloseAndOnBlur,
}}
ref={(node) => {
if (!node || !_.isFunction(this.props.innerRef)) {
ref={el => {
if (!_.isFunction(this.props.innerRef)) {
return;
}

this.props.innerRef(node);

// eslint-disable-next-line no-param-reassign
node.setNativeProps = this.setNativeProps;
this.props.innerRef(el);
}}
/>
);
Expand Down
21 changes: 21 additions & 0 deletions src/components/Picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 received the selected value as an argument */
onInputChange: PropTypes.func.isRequired,
};

const defaultProps = {
Expand All @@ -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, this.props.inputID);
return;
}

this.props.onInputChange(value, index);
Copy link
Member Author

@rushatgabhane rushatgabhane Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question from @parasharrajat - How is this change related to removing setNativeProps?

Rushat:

Anytime a Form input's value is changed, onInputChange() is called.

If you'll look at Form.js

App/src/components/Form.js

Lines 161 to 166 in 97ebb65

onInputChange: (value, key) => {
const inputKey = key || inputID;
this.setState(prevState => ({
inputValues: {
...prevState.inputValues,
[inputKey]: value,

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

This wasn't a problem before because Form previously set the value using ref.setNativeProps.
We're getting rid of that now. So we gotta fix it here.

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB).

If you look at Form, we have const inputKey = key || inputID; so if key (the 2nd param) is not provided, we will automatically use the inputID provided to that Picker. The only instance in which we want to pass a 2nd param is if that picker is also controlling the value for a different input. In which case we would provide a custom callback (not yet supported, see this issue) and pass the inputID of the second input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane what are your thoughts about my comment above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just started reading this, will get back soon

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB).

Thank you, I understand the use of 2nd parameter better now.
I've updated the call to not pass the 2nd parameter.

}

render() {
Expand All @@ -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]}>
Expand Down