[HOLD for payment 2022-05-24] [$1000] [HOLD for payment after regression testing] Refactor Picker to be compatible with Form

[luacmartins]

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:

1. An optional isFormInput prop.
2. If isFormInput is true, require a inputID prop. Use getInputIDPropTypes to enforce this propType rule.
3. Add an optional shouldSaveDraft prop. Defaults to false.
4. Make the value prop optional.
5. In the input’s onBlur method, call props.onBlur().
6. In the input’s onChange method (or equivalent e.g. onTextChange, etc), call props.onChange().
7. Remove the hasError prop.
8. Add an optional errorText prop. Defaults to an empty string.
9. Update the error message to display if errorText is truthy.
10. Update the inline error display style so it is left aligned with the label and input value. See example image for TextInput:
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().
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.
13. Remove any unused code.

There's an example of a refactor to TextInput in this PR.

[MelvinBot]

Triggered auto assignment to @sonialiap (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[MelvinBot]

Triggered auto assignment to @laurenreidexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[laurenreidexpensify]

https://www.upwork.com/jobs/~013e89828de19f7536

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

[MelvinBot]

Triggered auto assignment to @stitesExpensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[LucioChavezFuentes]

This issue looks self explanatory. May I take it as my task?

[rushatgabhane]

Sure, that'd be great. Looks like you're a new contributor, as a reminder make sure you go through CONTRIBUTING.md.

You could also #expensify-open-source slack channel if you like.

🎀👀🎀 C+ reviewed @LucioChavezFuentes wants to volunteer for this task.

cc: @stitesExpensify

[LucioChavezFuentes]

Thank you for the opportunity! In a few moments I will send my request to be invited to slack channel. I plan to post an update on issue progress on Monday 7th Frebuary 2022, is that ok?

[rushatgabhane]

@LucioChavezFuentes If @ stitesExpensify, gives you a go ahead for the PR. You can proceed with one

[LucioChavezFuentes]

My bad, I will proceed if approval.

[stitesExpensify]

Go for it @LucioChavezFuentes! Please apply for the job on Upwork and then @laurenreidexpensify will accept you

[MelvinBot]

📣 @LucioChavezFuentes You have been assigned to this job by @laurenreidexpensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[LucioChavezFuentes]

Thank you! I submitted my proposal on Upwork. I plan to finish the issue on Wednesday, 9th February.

[LucioChavezFuentes]

I had an issue rendering Picker component inside Form.stories.js. But I fix it by installing @storybook/addon-react-native-web as devDependency and add it on .storybook\main.js on addons array. The problem was related to No loader found for this type of file and it is documented in storybook, more info: https://github.com/storybookjs/addon-react-native-web/blob/main/README.md#common-issues. Is it OK to install this devDependecy?

packaje.json

.storybook\main.js

[luacmartins]

There's some info in the thread here https://expensify.slack.com/archives/C01GTK53T8Q/p1644243818897309. If we cannot fix the config, we can include this dev dependency.

[LucioChavezFuentes]

The issue is taking me longer than I expected so I don't think I can't finish until Friday. I hope I does not affect someone's agenda.

[rushatgabhane]

Friday is fine, thanks for letting us know!

[LucioChavezFuentes]

I encounter another issue, I can't get ref of Picker's dom element, looks like Picker from@react-native-picker/picker ( which is used inside RNPickerSelect from react-native-picker-select ) does not forward ref to its root element in Web version. ( according to @react-native-picker/picker documentation it does support in Android, don't know about IOS, I still haven't tested on these platforms ). I am still investigating on checking @react-native-picker/picker github's code and find out if there are newer versions which support this feature.

[luacmartins]

@LucioChavezFuentes can you pass a ref to Picker that exposes a focus() method?

[LucioChavezFuentes]

When I tried to pass ref on ref prop inside RNPickerSelect, I got RNPickerSelect class instance, not the input nor select element.

Inside BasePicker.js

Therefore...

When I pass ref to pickerProps, I got nothing.

Inside BasePicker.js

Am I missing something about Form implementation?

[LucioChavezFuentes]

I decided to check Picker and modify a little its forwardRef implementation inside mode_modules, and I finally got the DOM instance with focus( ) passing ref to pickerProps, it still does not show the picker's dropdown list when I click 'fix errors' but it proves Picker does not forward ref, at least at the version we are using.

Original code inside node_modules/@react-native-picker/picker/js/Picker.web.js

Modified code inside node_modules/@react-native-picker/picker/js/Picker.web.js

Passing ref to pickerProps:

DOM ref available:

handleOpen executes when click 'fix the errors', which is passed on onFocus pickerProps

What do you gus think?

[LucioChavezFuentes]

Guys another question, just to be sure. We are trying to get Picker's method focus( ) so we can open its dropdown list when user click on 'fix the erros' right?

[rushatgabhane]

We are trying to get Picker's method focus( ) so we can open its dropdown list when user click on 'fix the erros' right?

@LucioChavezFuentes No, focusing the Picker on errors shouldn't be the expected behaviour.

[luacmartins]

@LucioChavezFuentes once we click fix the errors the Picker should be scrolled into view (no animation). I don't think it's necessary for the dropdown to open.

[rushatgabhane]

once we click fix the errors the Picker should be scrolled into view

@luacmartins shouldn't that be implemented in the form?

[luacmartins]

Yes, it is! But it requires that the ref expose a focus() method.

[rushatgabhane]

@luacmartins Hmm, I could be wrong about this, just textInputRef.focus() shouldn't scroll to the input. https://stackoverflow.com/questions/57933647/how-to-scroll-into-view-in-react-native

Is scroll to errored input for works in the future?

I get that we need focus(), but not for scroll

[LucioChavezFuentes]

Thank you for the answer, right now I am testing with newer versions of Picker and see if it could provide me the ref.

[luacmartins]

@rushatgabhane hmm I remember testing it and having the input scroll into view on all devices. We are planning on using focus to simplify the implementation of scrolling into view.

[LucioChavezFuentes]

About scrolling on focus( ) I think it depends on tags or components you are trying to focus. On Web, Picker renders a <select> and I confirm executing focus ( ) on it does make a scroll. Meanwhile I tried to focus on a <div> and it doesn't make scroll.

[LucioChavezFuentes]

Guys, I don't think I can deliver a Pull Request on Friday but until next week. I apologize for the inconvenience.

[stitesExpensify]

That's fine @LucioChavezFuentes thanks for keeping us updated

[LucioChavezFuentes]

I can't get Picker's ref on Web because it is not exposed on this platform inside its library code, even in its latest version. So I made a pull request to @react-native-picker/picker with a fix to this issue, I hope they can review it soon. In any case, I am working on an alternative to scroll on Picker's input when click on 'fix the errors' on Web.

[LucioChavezFuentes]

I have another question. Should I create a Picker.stories.js inside stories directory so I can test it without being inside a Form?

[rushatgabhane]

Should I create a Picker.stories.js inside stories

@LucioChavezFuentes sure, go ahead that's one of the requirement anyway.

Does storybook also work to test the components in Android and IOS?

Nope

[LucioChavezFuentes]

Guys another question. Does storybook also work to test the components in Android, IOS or Desktop? If not, on what pages of App can I check to test Picker in these platforms?

[rushatgabhane]

As of now, storybook only works for web.

what pages of App can I check to test Picker in these platforms

Umm, Settings -> ProfilePage.

[LucioChavezFuentes]

Update, I only need to test my Picker on IOS and Mac, I am currently creating a Mac mini M1 instance on Scaleway.

[LucioChavezFuentes]

Guys I still haven't tested Picker inside Form besides on Storybook. Is there a Form component used somewhere in the App? I can not find page where Form is being used besides Storybook. Should I create a dummy page with Form to test?

[rushatgabhane]

@LucioChavezFuentes it's a WIP, so form isn't being used anywhere as of now.

Should I create a dummy page with Form to test?

I guess you can just pass isFormInput prop to the picker in Profile Page.

[LucioChavezFuentes]

Update. I finished my M1 instance config and making final tests on iOS and Desktop Mac. Also I made a draft pull request and I am updating its description. I hope to request review soon.

[LucioChavezFuentes]

By the way on Mac Desktop I am having trouble with CORS when I am trying to Login. It is the only platform it is hapenning. I have no .env file. Any suggestions?

[rushatgabhane]

By the way on Mac Desktop I am having trouble with CORS

@LucioChavezFuentes Try merging main from upstream. Also, this thread or this thread might help

I hope to request review soon.

Yep, looks like your PR for rn-picker is almost merged too. Thanks for the update, I'm excited!

[rushatgabhane]

Oops, looks like I unassigned myself by mistake 🫠 @stitesExpensify could you please assign me back. Thanks!

[luacmartins]

I deleted the last comment from Melvin since I just reassigned @rushatgabhane as C+ to this issue.

[rushatgabhane]

Thanks so much

[LucioChavezFuentes]

Guys, I made the request for a review to my pull request. Please, check my changes. I want to know your opinion and I will gladly to make any changes if something doesn't seem right. I keep an eye about the pull request on @react-native-picker/picker and see if future changes should be made in case this library gets updated.

[LucioChavezFuentes]

Update The merge of the pull request on @react-native-picker/picker is complete. I will update my pull request to App.

[LucioChavezFuentes]

Guys, I updated my branch from main origin App and now I having trouble running storybook. Is there any breaking change I should be aware of?

[rushatgabhane]

Likewise. @LucioChavezFuentes I'd suggest that you post it on slack