Refactor AddressSearch to be compatible with Form by kakajann · Pull Request #7701 · Expensify/App

kakajann · 2022-02-11T05:02:06Z

Details

An optional isFormInput prop.

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

Add an optional shouldSaveDraft prop. Defaults to false.

Done.

Make the value prop optional

It was already optional.

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

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

Done.

Remove the hasError prop

There was no hasError prop.

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

Update the error message to display if errorText is truth.

Done.

Update the inline error display style so it is left aligned with the label and input value. See example image for TextInput

I don't think it's my concern. AdressSearch is using TextInput

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().

Done.

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.

Done.

Remove any unused code.

Nothing to remove.

Fixed Issues

$ #7538

Tests

Verify that no errors appear in the JS console

QA Steps

Nothing to test I guess. It's only refactor.

Verify that no errors appear in the JS console

Tested On

Screenshots

rushatgabhane

@kakajann two things -

Error from storybook - props.onChange isn't a function.
Point 10 isn't implemented - Error text needs to be aligned with label and input value as show here - #7538 (comment)

kakajann · 2022-02-14T13:30:07Z

Error from storybook - props.onChange isn't a function.

I'll take a look at this soon.

Point 10 isn't implemented - Error text needs to be aligned with label and input value as show here - #7538 (comment)

As I mentioned above, AddressSearch is using TextInput. I didn't want to modify it. It should have been done in this PR

Also, this PR covers the error alignment: https://github.com/Expensify/App/pull/7627/files

Do you still want me to modify error alignment?

…dress-search

kakajann · 2022-02-14T14:43:33Z

@rushatgabhane

Error from storybook - props.onChange isn't a function

is fixed

luacmartins

Left a few comments. I also see this error in Storybook when typing into the field (possibly related to my comment about onChange)

src/components/AddressSearch.js

luacmartins · 2022-02-14T15:36:48Z

src/components/AddressSearch.js

+                inputID: props.inputID,
+                shouldSaveDraft: props.shouldSaveDraft,
+                onBlur: props.onBlur,
+                onChange: (text) => {


Maybe I'm wrong, but I believe that onChanteText passes the text value as argument vs onChange passes an event. We should make sure that we are passing the text value here.

I think it returns only text value, considering AddressSearch is using TextInput as the base component.

App/src/components/TextInput/BaseTextInput.js

Line 240 in c049cc4

onChangeText={this.setValue}

App/src/components/TextInput/BaseTextInput.js

Lines 121 to 128 in c049cc4

setValue(value) {

if (this.props.onChange) {

this.props.onChange(value);

}

this.value = value;

Str.result(this.props.onChangeText, value);

this.activateLabel();

}

Hmm that is onChangeText though, not onChange. I tried logging the text value here and I see the following:

Is that what we expect?

I don't think so.
Should I push this diff?

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js index 005438f75..bee2e40cb 100644 --- a/src/components/AddressSearch.js +++ b/src/components/AddressSearch.js @@ -158,7 +158,7 @@ const AddressSearch = (props) => { inputID: props.inputID, shouldSaveDraft: props.shouldSaveDraft, onBlur: props.onBlur, - onChange: (text) => { + onChangeText: (text) => { if (skippedFirstOnChangeTextRef.current) { props.onChange({street: text}); } else {

@kakajann Sorry for the late reply, I was ooo for most of last week. I think it's fine to keep onChangeText as we had before. The onChange handler will be passed by the parent Form component.

@luacmartins, done

kakajann · 2022-02-15T14:10:55Z

Left a few comments. I also see this error in Storybook when typing into the field (possibly related to my comment about onChange)

Is this error occurs on the Form stories or the AddressSearch?

luacmartins · 2022-02-15T17:02:47Z

Hmm I think that error is caused by a regression in TextInput. I already commented in the offending PR so hopefully we can get it fixed soon.

…dress-search

luacmartins

Thanks for the changes @kakajann! I believe since props.value can be undefined, we should add a check here so we don't try to access .trim() on undefined.

kakajann · 2022-02-23T15:44:18Z

@luacmartins Done

luacmartins

Sorry for all the back and forth. Left a few more comments.

src/stories/AddressSearch.stories.js

luacmartins · 2022-02-23T16:07:22Z

src/components/AddressSearch.js

+     * @param {Object} props - props passed to the input
+     * @returns {Object} - returns an Error object if isFormInput is supplied but inputID is falsey or not a string
+     */
+    inputID: props => FormUtils.getInputIDPropTypes(props),


We renamed this util function to validateInputIDProps. Please update it.

Co-authored-by: Carlos Martins <luacmartins@gmail.com>

kakajann · 2022-02-23T16:17:56Z

No problem @luacmartins

luacmartins

LGTM! Thanks for all the changes @kakajann! @rushatgabhane and @marcochavezf all yours.

rushatgabhane · 2022-02-24T19:37:08Z

Thanks for the review @luacmartins 🙇

rushatgabhane

@kakajann could you please merge main into your branch.

@marcochavezf LGTM! 🎉️
Tests well, and I gave this a run after merging main from upstream.

kakajann · 2022-02-24T20:10:49Z

@rushatgabhane Done

OSBotify · 2022-02-24T21:34:41Z

✋ 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-03-01T19:54:26Z

🚀 Deployed to staging by @marcochavezf in version: 1.1.41-0 🚀

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

OSBotify · 2022-03-02T10:47:44Z

🚀 Deployed to staging by @marcochavezf in version: 1.1.41-0 🚀

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

OSBotify · 2022-03-11T00:04:17Z

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

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

kakajann added 3 commits February 11, 2022 09:50

AddressSearch component refactor

5fcb7b7

update form stories

255d268

create story for AddressSearch

d699187

kakajann requested a review from a team as a code owner February 11, 2022 05:02

MelvinBot requested review from marcochavezf and rushatgabhane and removed request for a team February 11, 2022 05:02

kakajann changed the title ~~Refactor AddressForm to be compatible with Form~~ Refactor AddressSearch to be compatible with Form Feb 11, 2022

luacmartins self-requested a review February 14, 2022 11:25

rushatgabhane suggested changes Feb 14, 2022

View reviewed changes

kakajann added 2 commits February 14, 2022 19:25

Merge branch 'main' of github.com:kakajann/Expensify into refactor/ad…

4387358

…dress-search

fix props.onChange is not a function error

e868e06

luacmartins requested changes Feb 14, 2022

View reviewed changes

kakajann added 2 commits February 15, 2022 18:51

remove unneccary comment

2219e26

make default value undefined

3cebf96

kakajann added 3 commits February 16, 2022 19:54

Merge branch 'main' of github.com:kakajann/Expensify into refactor/ad…

c2b4ce5

…dress-search

Merge branch 'main' of github.com:kakajann/Expensify into refactor/ad…

fbee850

…dress-search

change onChange prop to onChangeText

996f12b

kakajann requested review from luacmartins and rushatgabhane February 23, 2022 11:18

luacmartins reviewed Feb 23, 2022

View reviewed changes

validate props.value before trimming it

7186e17

kakajann requested a review from luacmartins February 23, 2022 15:44

luacmartins reviewed Feb 23, 2022

View reviewed changes

Update src/stories/AddressSearch.stories.js

9217a76

Co-authored-by: Carlos Martins <luacmartins@gmail.com>

rename getInputIDPropTypes with validateInputIDProps

2392560

luacmartins approved these changes Feb 23, 2022

View reviewed changes

rushatgabhane approved these changes Feb 24, 2022

View reviewed changes

Merge branch 'Expensify:main' into refactor/address-search

15926e2

marcochavezf approved these changes Feb 24, 2022

View reviewed changes

marcochavezf merged commit 1ab42ab into Expensify:main Feb 24, 2022

OSBotify mentioned this pull request Mar 1, 2022

Deploy Checklist: New Expensify 2022-03-01 #7954

Closed

68 tasks

botify mentioned this pull request Mar 10, 2022

[HOLD for payment 2022-03-17] Refactor AddressSearch to be compatible with Form #7538

Closed

kakajann deleted the refactor/address-search branch March 11, 2022 00:12

kakajann restored the refactor/address-search branch March 11, 2022 00:12

Stutikuls mentioned this pull request Mar 16, 2022

Implement infinite sessions #7

Merged

luacmartins mentioned this pull request May 17, 2022

Fix AddressSearch integration with Form #9059

Closed

kakajann mentioned this pull request Sep 1, 2022

[HOLD for payment 2022-11-02] [$1000] [Form Refactor] AddressForm #10738

Closed

8 tasks

	setValue(value) {
	if (this.props.onChange) {
	this.props.onChange(value);
	}
	this.value = value;
	Str.result(this.props.onChangeText, value);
	this.activateLabel();
	}

Conversation

kakajann commented Feb 11, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Uh oh!

rushatgabhane left a comment

Choose a reason for hiding this comment

Uh oh!

kakajann commented Feb 14, 2022

Uh oh!

kakajann commented Feb 14, 2022

Uh oh!

luacmartins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kakajann commented Feb 15, 2022

Uh oh!

luacmartins commented Feb 15, 2022

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

kakajann commented Feb 23, 2022

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kakajann commented Feb 23, 2022

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

rushatgabhane commented Feb 24, 2022

Uh oh!

rushatgabhane left a comment

Choose a reason for hiding this comment

Uh oh!

kakajann commented Feb 24, 2022

Uh oh!

OSBotify commented Feb 24, 2022

Uh oh!

OSBotify commented Mar 1, 2022

Uh oh!

OSBotify commented Mar 2, 2022

Uh oh!

OSBotify commented Mar 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

luacmartins left a comment •

edited

Loading