7535 refactor picker compatible form

[LucioChavezFuentes]

With the implementation of our new Form component we need to refactor Picker to be Form compatible. Here are the changes that need to be made:

Details

1. An optional isFormInput prop.

Finish.

---

2. If isFormInput is true, require a inputID prop. Use getInputIDPropTypes to enforce this propType rule.

Finish. Console will return error if isFormInput is settled to true but inputID is not settled at all.

---

3. Add an optional shouldSaveDraft prop. Defaults to false.

Finish. But...
I find an issue, (or expected behaviour?) I think is related to Form component.

I run Storybook and I go to Form tab and select Default. When I write new values on Inputs inside Form and I then click on submit button, only updated values inside Inputs with shouldSaveDraft prop settled to true are submitted. The other Inputs only submit the dafult value. not the values showed in the UI.

Should I attend this issue too? Is being attended by someone else? Or is this the expected behaviour?
It happens on TextInput and I see it too on Picker.

---

4. Make the value prop optional.

Finish.

---

5. In the input’s onBlur method, call props.onBlur().

Finish.

---

6. In the input’s onChange method (or equivalent e.g. onTextChange, etc), call props.onChange().

Finish. I created a handleChange function and put it on onTextChange. It manages props.onChange and state.selectValue value of RNPickerSelect so it can change the value on UI.

---

7. Remove the hasError prop.

Finish. Though I leave a hasError variable based on errorText on BasePicker. I think is easier to read hasError than !_.isEmpty(this.props.errorText) on style logic.

---

8. Add an optional errorText prop. Defaults to an empty string.

Finish.

---

9. Update the error message to display if errorText is truthy.

Finish. Created hasError variable explained on point 7. Apparently it is only used in style logic.

---

10. Update the inline error display style so it is left aligned with the label and input value.

Finish.

---

11. Make sure that props.ref is attached to the appropriate DOM node. This could involve forwarding the ref to a child component. This is important so we can call ref.focus().

Well, this was a tricky one. ref seems not to be exposed on all platforms.

Web & Mobile Web:
Picker from https://github.com/react-native-picker/picker is being used inside RNPickerSelect. Picker ref prop does not work on Web therefore it can not get <select>'s ref that Picker renders, so a pull request has been made with a potential fix to this issue. In any case, I implemented an alternative in /BasePicker/index.js to scroll to Picker on click fix the errors. If Picker returns a ref, the implementation of componentDidMount inside /BasePicker/index.js and the move of base logic to /BasePicker/index.native.js would not be neccesary.

Desktop (mac):
Teorically works similar to Web, though I haven't tested the scroll on fix the errors on this platform.

Android:
Picker does return a focus() method but I test it with @react-native-picker/picker on version 2.2.1. This library needs to be updated at least to 1.16.0 according to documentation in order to focus() method be exposed.

iOS: Same situation as Android. @react-native-picker/picker must be updated to expose ref with focus() method.

---

12. Add the input to the test Form stories and make sure that it works. You can check this by running npm run storybook and testing the component in the example form.

Finish. I guess Storybook is the only way to see how Picker performs inside Form component. I haven't tested Form functionalities on Picker like scroll on click fix the errors on any other platform besides Storybook.

---

13. Remove any unused code.

Finish. My lint doesn't recognize any unused variable.

---

Extra.

Created a Picker.stories.js for Storybook to test Picker alone.

---

Posible Issues Found

1. Only the values on inputs inside Form with shouldSaveDraft set to true are submitted.
2. The sidebar of storybook in Mobile Web have a "white" background that prevents to see text clearly.

Fixed Issues

$ #7535

Tests

Make sure refactor changes does not break the current functionality of Picker on all platforms:

Web, Mobile Web, Desktop, Android and iOS:

1. Go to Settings -> ProfilePage.
2. Click on Prefered pronouns Picker to focus. A list of options should appear.
3. Select an option.
4. The picker input value showed should change to the selected option.
5. Click save button.
6. Exit Profile Page and enter again.
7. The value previously selected and saved should populate the Picker input.
8. Select another option on Prefered pronouns Picker.
9. Exit Profile Page without save.
10. Return to Profile Page. The most recent Saved change value should populate Picker not the most recent change.

---

Storybook Web and Web Mobile (Google Chrome for Android)

Test Picker behaviour inside Form in Storybook

1. Run npm run storybook.
2. Go to Form --> Default
3. Change Picker values. The UI should change accordingly
4. Click submit button
5. An Alert window with JSON with input's submitted values should appear.
6. Note the JSON only show the updated values on inputs with "shouldSaveDraft" in Form.stories.js (Expected Behaviour?)

Test scroll to Picker on click fix the errrors in Storybook.

1. Go to Form.stories.js,
2. Go to InputError.args and set non empty values as default to other input types besides Pickers
3. Run storybook with npm run storybook
4. On Storybook, go to Form --> Input Error
5. Add more inputs to Form.stories.js or shrink window enough to get the first Picker out of view while we can see submit button.
6. Click submit button. Picker error(s) and fix the errors sentence should appear.
7. Click fix the errors. The view should scroll and center to the first Picker on error.

• Verify that no errors appear in the JS console

---

QA Steps

Android and iOS:
The focus() method should be checked if it does the expected behaviour on Form component.
I only check if focus() exist with a console.log(this.pickerRef.focus) inside componentDidUpdate in ProfilePage.js but couldn't test its behaviour beacuse absecence of Form component in App.

• Verify that no errors appear in the JS console

Tested On

StoryBook Form tab and App inside ProfilePage.js

• Web
• Mobile Web

Only on App inside ProfilePage.js

• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[LucioChavezFuentes]

I have read the CLA Document and I hereby sign the CLA

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by @stitesExpensify in version: 1.1.54-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[mvtglobally]

@luacmartins @rushatgabhane @parasharrajat @stitesExpensify Can someone help out with the QA steps on this PR? Are we able to check it or this will be Internal?

[luacmartins]

@mvtglobally please follow the steps listed under Tests:

Web, Mobile Web, Desktop, Android and iOS:

1. Go to Settings -> ProfilePage.
2. Click on Prefered pronouns Picker to focus. A list of options should appear.
3. Select an option.
4. The picker input value showed should change to the selected option.
5. Click save button.
6. Exit Profile Page and enter again.
7. The value previously selected and saved should populate the Picker input.
8. Select another option on Prefered pronouns Picker.
9. Exit Profile Page without save.
10. Return to Profile Page. The most recent Saved change value should populate Picker not the most recent change.

[chiragsalian]

Posting here for awareness. The commit 01fa795 from this issue caused this deployblocker #8570. I don't think revert is needed just yet if we can figure out the solution. I'm not sure of the solution just yet as i started investigating.

[LucioChavezFuentes]

Posting here for awareness. The commit 01fa795 from this issue caused this deployblocker #8570. I don't think revert is needed just yet if we can figure out the solution. I'm not sure of the solution just yet as i started investigating.

@chiragsalian I fixed it, you need to provide key to Picker on ProfilePage.js

In this case I set the state on key but it could be anything that reference the auto time zone of the user. The problem exists because I made a derivated state from props on BasePicker.js so it could recieve a default or draft value from Form but making state from props will make component to ignore next changes on props. To update the component is useful set key so React update component changes.

More info here: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key

[rushatgabhane]

@LucioChavezFuentes Looks like you have found a fix. This PR is under regression period, so could you please create a PR that fixes the deploy blocker? Thanks!

[parasharrajat]

I don't think #7807 (comment) is a good idea. the key will cause remounting. It will make a trivial component expensive.

[rushatgabhane]

I set the state on key but it could be anything

@LucioChavezFuentes @parasharrajat can't we just set the key to something like "timezonePicker" and call it a day?

Or the key needs to change to force remounting (bad solution)?

It's a deploy blocker so let's get it fixed asap! :)

[chiragsalian]

I don't think setting the key to something like "timezonePicker" would work.
While the proposed solution is not ideal i'll still go ahead and push it out in a PR and CP it so that it does not block deploy.

[chiragsalian]

Meanwhile, any chance you guys can continue and discuss a more ideal solution and implement it when possible? 🙇‍♂️

[luacmartins]

Thanks for investigating and fixing this @chiragsalian! It seems that the fix was merged and deployed.

@LucioChavezFuentes would you mind investigating other solutions that solve this problem without causing a remount?

[LucioChavezFuentes]

@LucioChavezFuentes would you mind investigating other solutions that solve this problem without causing a remount?

Sure. I'm on it.

[LucioChavezFuentes]

I found a better solution. We need to do the following:

• inside BasePicker/index.js : Check if props.value is undefined or equal to prevProps.value or equal to Picker's state.selectedValue on componentDidUpdate, skip update if at least one condition comply.

• inside basePickerPropTypes.js: Set undefined as default to prop value in basePickerPropTypes.js

But eslint does not allow me to set state inside componentDidUpdate, could it be an exception? I saw another similar exception inside BaseTextInput

inside src/components/Picker/BasePicker/index.js

inside src/components/Picker/BasePicker/basePickerPropTypes.js

[parasharrajat]

I don't have the context why we changed the implementation of Picker here.

    constructor(props) {
        super(props);

        this.state = {
            selectedValue: this.props.value || this.props.defaultValue,
        };

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

    updateSelectedValueAndExecuteOnChange(value) {
        this.props.onInputChange(value);
luacmartins marked this conversation as resolved.
        this.setState({selectedValue: value});
    }

But instead of using #7807 (comment), you should directly pass the this.props.value to RNPickerSelect like it used to.

[OSBotify]

🚀 Deployed to production by @Julesssss in version: 1.1.54-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[LucioChavezFuentes]

Just passing props.value to RNPickerSelect doesn't make Picker work correctly inside Form on Storybook.

I added logic to Picker so it could work inside Form whitout breaking Picker functionality as stand alone component. I based on TextInput's implementation.

I made a profile of Picker's performance comparing between this pull request merge (no key) and componentDidUpdate proposal and I conclude it does not add dramatic overhead as key implementation does on Picker and ProfilePage.

Profile on one render:

ProfilePage performance on 3 renders:

	Render	Current Merge (no key)	ComponentDidUpdate Proposal
	1	106.3 ms	114.1 ms
	2	101 ms	102.3 ms
	3	109.5 ms	109.2 ms
Average		105.6 ms	108.53 ms

Picker Timezone performance on 3 renders:

	Render	Current Merge (no key)	ComponentDidUpdate Proposal
	1	63.7 ms	70.1 ms
	2	63.6 ms	62.3 ms
	3	63.1 ms	68.6 ms
Average		63.46 ms	67 ms

But, after all said, I must say @parasharrajat is right, Picker shouldn't have that complexity.

I have an idea that might be a better solution but I need time to make tests. I'll post when I have something.
I posted componentDidUpdate proposal as an urgent better solution than key implementation.

[luacmartins]

Thanks for the detailed explanation @LucioChavezFuentes! Looking forward to your revised proposal!

[luacmartins]

@LucioChavezFuentes I just noticed that this is also affecting StatePicker. It isn't updated when selecting an address via the AddressSearch input. Adding a key to BasePicker solves the issue.

Steps to reproduce:

1. Navigate to Settings > Payments > Add payment method > Debit card
2. Search for an address and select it
3. Notice that all fields are filled automatically, except for State.

The same is true for other usages of StatePicker in conjunction with AddressSearch.

[chiragsalian]

Looks like the temporary fix i added (as per this suggestion) ended up creating another deploy blocker issue.

Investigating but any suggestions or a more permanent solution are welcome too 🙂

[LucioChavezFuentes]

@chiragsalian @luacmartins I found a better solution:

class BasePicker extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
--             selectedValue: this.props.value || this.props.defaultValue,
++            selectedValue: this.props.defaultValue,
        };

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

    updateSelectedValueAndExecuteOnChange(value) {
        this.props.onInputChange(value);
        this.setState({selectedValue: value});
    }

    render() {
        const hasError = !_.isEmpty(this.props.errorText);
        return (
            <RNPickerSelect
                onValueChange={this.updateSelectedValueAndExecuteOnChange}
                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.state.selectedValue}
++           value={this.props.value || this.state.selectedValue}
                Icon={() => this.props.icon(this.props.size)}
                disabled={this.props.disabled}
                fixAndroidTouchableBug
                onOpen={this.props.onOpen}
                onClose={this.props.onClose}
                pickerProps={{
                    onFocus: this.props.onOpen,
                    onBlur: this.props.onBlur,
                    ref: this.props.innerRef,
                }}
            />
        );
    }
}

and key won't be neccesary to update on click checkbox on ProfilePage

[luacmartins]

Thanks for the new solution @LucioChavezFuentes! I ran some quick tests and it seems to work well. Would you mind putting a PR up? We will go through the regular process to get this well tested and make sure we don't have any more blockers

[LucioChavezFuentes]

Sure, Im on it.

[LucioChavezFuentes]

PR of my last proposal is up!