statepicker refactored to compatible with form #8758

jayeshmangwani · 2022-04-23T07:39:28Z

Details

Fixed Issues

$ #7537

Tests

Go to Add debit card page under, Settings > Payments > Add Payment method
press save without selecting a state from the StatePicker
we will get an error message for the state picker (before there were no errors)
Now press on the StatePicker, we will able to see states list and select any state from the list , it should work (before it was not selectable onChange was not working before changes)

Test under storybook

run storybook using npm run storybook
Select Form from the LHS
Select InputError from the options
Last option is StatePicker with label State , and we can test here working of the picker (select , update , errorText)

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Go to Add debit card page under, Settings > Payments > Add Payment method
press save without selecting a state from the StatePicker
we will get an error message for the state picker
Now press on the StatePicker, we will able to see states list and select any state from the list , it should work

Verify that no errors appear in the JS console

Screenshots

Web

After-7537.mov

Mobile Web

mWeb.mov

Desktop

8758-Desktop.mov

iOS

8758-iOs.mov

Android

8758-Android.mp4

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

melvin-bot · 2022-04-23T07:39:31Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

github-actions · 2022-04-23T07:39:44Z

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

parasharrajat

Please attach test videos for all platforms and sign the CLA.

Missing QA steps.

src/components/StatePicker.js

parasharrajat · 2022-04-24T17:29:15Z

src/components/StatePicker.js

        value={props.value}
        label={props.label || props.translate('common.state')}
        hasError={props.hasError}
        errorText={props.errorText}
+        onBlur={props.onBlur}
+        containerStyles={props.containerStyles}


Did you define this prop? I don't think we need this.

Yes I have defined this, onBlur was in task's requirement, and containerStyles added for styling the picker

I don't see the prop definitions for these.

yes you are right, I have not defined containerStyles in props, and it was also not included in the task to add, so I think I should remove containerStyles from here

parasharrajat · 2022-04-24T17:49:21Z

src/components/StatePicker.js

        placeholder={{value: '', label: '-'}}
        items={STATES}
-        onInputChange={props.onChange}
+        onInputChange={props.onInputChange}
        value={props.value}
        label={props.label || props.translate('common.state')}
        hasError={props.hasError}


This should be removed. Right?

I think we have to update the name only from onChange to onInputChange, if i am removing this then picker is not working

I meant the hasError. Review comments refer to the line on which they are tagged.

I have removed hasError from props only, let me remove hasError from here too

parasharrajat · 2022-04-24T17:51:12Z

src/components/StatePicker.js

+
+    /** Error text to display */
+    errorText: PropTypes.string,
+
    ...withLocalizePropTypes,
 };

 const defaultProps = {
    label: '',
    value: '',


Maybe pass undefined. '' is still a value. I am not sure if Picker checks for truthy value before binding it.

an empty string will work here, we are checking this at 2 places

App/src/components/InlineErrorText.js

Lines 20 to 22 in ad637d3

if (_.isEmpty(props.children)) {

return null;

}

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

Line 26 in ad637d3

const hasError = !_.isEmpty(this.props.errorText);

I am talking about the value.

yes value will be undefined here

jayeshmangwani · 2022-04-25T04:48:06Z

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

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

…ani/App into refactor_statepicker

parasharrajat · 2022-04-25T11:18:23Z

src/pages/ReimbursementAccount/CompanyStep.js

                            value={this.state.incorporationState}
+                            errorText={this.getErrorText('incorporationState')}
                            hasError={this.getErrors().incorporationState}


Shouldn't we remove these?

Thanks for pointing out these changes, Yes I need to change all these places where we are using StatePicker,

App/src/pages/ReimbursementAccount/AddressForm.js

Line 75 in ad637d3

hasError={Boolean(props.errors.state)}

App/src/pages/ReimbursementAccount/CompanyStep.js

Line 292 in ad637d3

hasError={this.getErrors().incorporationState}

parasharrajat

Did you run npm run lint? Can you please proactively test your changes? Please ping me when you think it's fully ready.

luacmartins

A few comments:

Test steps seem to be incomplete.
Video for mWeb is 0s long.
There are a few lint errors:

luacmartins · 2022-04-25T17:51:53Z

src/pages/ReimbursementAccount/AddressForm.js

@@ -72,7 +72,6 @@ const AddressForm = props => (
                    value={props.values.state}
                    onChange={value => props.onFieldChange({state: value})}


We have to rename onChange to onInputChange

luacmartins · 2022-04-25T18:08:27Z

src/pages/ReimbursementAccount/CompanyStep.js

                            value={this.state.incorporationState}
-                            hasError={this.getErrors().incorporationState}
+                            errorText={this.getErrorText('incorporationState')}


The incorporationState key is not defined here.

luacmartins · 2022-04-25T18:10:47Z

src/stories/Form.stories.js

+                inputID="pickState"
+                shouldSaveDraft
+                isFormInput
+            />


Please add a top margin like the other inputs

we have not defined a style prop for StatePicker, we need to create containerStyles prop here

or you can just wrap it in a View like we do in all calls of StatePicker, e.g.

App/src/pages/ReimbursementAccount/CompanyStep.js

Lines 287 to 294 in fdd741f

<View style={styles.mt4}>

<StatePicker

label={this.props.translate('companyStep.incorporationState')}

onChange={value => this.clearErrorAndSetValue('incorporationState', value)}

value={this.state.incorporationState}

hasError={this.getErrors().incorporationState}

/>

</View>

src/components/StatePicker.js

luacmartins · 2022-04-25T18:40:40Z

We should also define StatePicker here

jayeshmangwani · 2022-04-25T20:08:57Z

A few comments:

Test steps seem to be incomplete.

Video for mWeb is 0s long.

There are a few lint errors:

tried npm run test, its working, then I tried npm run lint but it is stuck there in command line I have waited 3-4 hours but lint was not showing error nor success message
video updated
lint errors solved

luacmartins · 2022-04-25T20:47:13Z

tried npm run test, its working, then I tried npm run lint but it is stuck there in command line I have waited 3-4 hours but lint was not showing error nor success message

I mean the Test steps in your PR description, i.e.:

Go to Add debit card

press save without selecting state from the picker

we will get an error message for state picker

jayeshmangwani · 2022-04-26T08:16:04Z

tried npm run test, its working, then I tried npm run lint but it is stuck there in command line I have waited 3-4 hours but lint was not showing error nor success message

I mean the Test steps in your PR description, i.e.:

Go to Add debit card

press save without selecting state from the picker

we will get an error message for state picker

Thank you for the update,

I have updated the description please check whenever you get the time and please let me know if I have missed any point as it's my first PR in the repo.

parasharrajat

The code looks good and tests well. You will have to add the QA steps before we can merge this. @jayeshmangwani

Note: QA team can't run run storybook using npm run storybook so use Go to https://staging.new.expensify.com/docs/index.html for storybook

cc: @luacmartins

PR Reviewer Checklist

🎀 👀 🎀 C+ reviewed

jayeshmangwani · 2022-04-26T13:54:17Z

@parasharrajat added QA steps

luacmartins

Thanks for the changes. Left one more comment.

luacmartins · 2022-04-26T16:22:09Z

src/stories/Form.stories.js

@@ -198,6 +210,7 @@ InputError.args = {
        accountNumber: '',
        pickFruit: '',
        pickAnotherFruit: '',
+        pickState: '',


Can we also add a non-empty default value for pickState here?

During my testing, the default value didn't seem to work properly on page load.

storybook.mov

I think we have to add 1 more prop in StatePicker for this change, we need to pass defaultValue to make it work

I have added a default value for pickState, but we also have to pass defaultValue in StatePicker component, please let me know your thought on this

That sounds similar to what I'm doing in the Datepicker refactor. If that's the case I think it's fine :)

Okay, then its fine

Hmm the default value still doesn't load for me. Can you please check?

default.mov

@luacmartins we need to pass defaultValue as a prop in StatePicker here

https://github.com/jayeshmangwani/App/blob/a8d30f4de9ac1638f45eae693710b3069aa7f4f3/src/components/StatePicker.js#L42-L44

defaultValue={props.defaultValue}

as this value is being used in the BasePicker

https://github.com/jayeshmangwani/App/blob/a8d30f4de9ac1638f45eae693710b3069aa7f4f3/src/components/Picker/BasePicker/index.js#L14

parasharrajat · 2022-04-27T18:15:34Z

@jayeshmangwani Please merge main as there are conflicts. Thanks.

jayeshmangwani · 2022-04-27T18:46:10Z

@parasharrajat latest main branch merged,
@luacmartins I have added an extra field in StatePicker for defaultValue, please check this, it is working now

parasharrajat · 2022-04-27T20:49:26Z

I can't test it. The storybook is not working for me. https://expensify.slack.com/archives/C01GTK53T8Q/p1651081332743029

Possible solution https://expensify.slack.com/archives/C01GTK53T8Q/p1651082528577659?thread_ts=1651081332.743029&cid=C01GTK53T8Q

jayeshmangwani · 2022-04-27T20:55:14Z

Yes this solution is working for me, I have tested in storybook

luacmartins · 2022-04-27T21:30:33Z

src/components/StatePicker.js

+    shouldSaveDraft: false,
+    isFormInput: false,
+    inputID: undefined,
+    onBlur: () => {},


Hmm I see this console warning:

it's solved now, now tested the app in the web and no error is there and picker is working

tested in storybook , its working fine and no console error

storybook-test.mov

luacmartins · 2022-04-27T21:31:43Z

src/components/StatePicker.js

        value={props.value}
+        defaultValue={props.defaultValue}


I think we should pass either value or defaultValue but not both.

now added conditional value here, if the value is available then the value will be used otherwise defaultValue

parasharrajat

LGTM.

luacmartins

LGTM and tests well. Thanks for the work @jayeshmangwani

OSBotify · 2022-04-28T18:36:14Z

✋ 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 · 2022-05-09T20:04:27Z

🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-05-13T16:13:23Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

statepicker refactored to compatible with form

ee883a3

jayeshmangwani requested a review from a team as a code owner April 23, 2022 07:39

melvin-bot bot requested review from luacmartins and parasharrajat and removed request for a team April 23, 2022 07:39

parasharrajat suggested changes Apr 24, 2022

View reviewed changes

Update src/components/StatePicker.js

0aba0ff

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

jayeshmangwani requested a review from parasharrajat April 25, 2022 07:12

jayeshmangwani added 2 commits April 25, 2022 16:40

Removed hasError & containerStyles, changed value to undefined

339d441

Merge branch 'refactor_statepicker' of https://github.com/jayeshmangw…

9e93cda

…ani/App into refactor_statepicker

parasharrajat suggested changes Apr 25, 2022

View reviewed changes

removed hasError prop from AddressForm & CompanyStep

cee55cb

jayeshmangwani requested a review from parasharrajat April 25, 2022 11:28

parasharrajat suggested changes Apr 25, 2022

View reviewed changes

luacmartins reviewed Apr 25, 2022

View reviewed changes

updated default props in StatePicker, defined incorporationState

3a09b5c

parasharrajat previously approved these changes Apr 26, 2022

View reviewed changes

luacmartins reviewed Apr 26, 2022

View reviewed changes

added default value for pickState

a8d30f4

jayeshmangwani dismissed parasharrajat’s stale review via a8d30f4 April 26, 2022 18:23

added defaultValue prop to StatePicker

cae92e8

jayeshmangwani added 2 commits April 27, 2022 23:47

Merge branch 'main' into refactor_statepicker

7597a8a

merged & resolved conflicts

2349a7c

luacmartins reviewed Apr 27, 2022

View reviewed changes

added conditional value to StatePicker

dcdb112

parasharrajat approved these changes Apr 28, 2022

View reviewed changes

luacmartins approved these changes Apr 28, 2022

View reviewed changes

luacmartins merged commit db6fab7 into Expensify:main Apr 28, 2022

OSBotify mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

kbecciv mentioned this pull request May 11, 2022

IOS - Payments - State field doesn't have blue border. #8932

Closed

melvin-bot bot mentioned this pull request May 13, 2022

[HOLD for payment 2022-05-20] [$500] Refactor StatePicker to be compatible with Form #7537

Closed

		@@ -72,7 +72,6 @@ const AddressForm = props => (
		value={props.values.state}
		onChange={value => props.onFieldChange({state: value})}

	<View style={styles.mt4}>
	<StatePicker
	label={this.props.translate('companyStep.incorporationState')}
	onChange={value => this.clearErrorAndSetValue('incorporationState', value)}
	value={this.state.incorporationState}
	hasError={this.getErrors().incorporationState}
	/>
	</View>

statepicker refactored to compatible with form #8758

statepicker refactored to compatible with form #8758

Conversation

jayeshmangwani commented Apr 23, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

melvin-bot bot commented Apr 23, 2022

github-actions bot commented Apr 23, 2022 • edited Loading

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayeshmangwani commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins commented Apr 25, 2022

jayeshmangwani commented Apr 25, 2022

luacmartins commented Apr 25, 2022

jayeshmangwani commented Apr 26, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

jayeshmangwani commented Apr 26, 2022

luacmartins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Apr 27, 2022

jayeshmangwani commented Apr 27, 2022

parasharrajat commented Apr 27, 2022 • edited Loading

jayeshmangwani commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 28, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

jayeshmangwani commented Apr 23, 2022 •

edited

Loading

github-actions bot commented Apr 23, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

luacmartins Apr 25, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat commented Apr 27, 2022 •

edited

Loading