17548: Personal details push to page #18424

gedu · 2023-05-04T17:49:54Z

Details

Updated dropdowns fields to be MenuItems which sends the user to another page to pick the option
Standarized the spaces between Inputs, created a new FormSpace component.

Fixed Issues

$ #17548
PROPOSAL: #17548 (comment)

Tests

Errors on Empty State

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Save
All the Inputs even the Menu Items should show a required field error

Update the menu Country item (no USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select a new country (no USA)
The Country Item should show the selected Country
The State Input should be an input component
You can select new ones and the Menu Item should be updated

Update the menu Country item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
Should send you back and the Country Item should show the selected Country
The State Input should be a Menu Item

Update the menu State item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
The Country Item should show the selected Country
Clicking on the State menu should send you to a new Page
You can select any State
Should send you back to and show you the updated State

Autocomplete address

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Start filling the Address Line 1, a suggestion menu should shows up
Clicking on any address should autocomplete, menu items and inputs

Verify that no errors appear in the JS console

Offline tests

Errors on Empty State

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Save
All the Inputs even the Menu Items should show a required field error

Update the menu Country item (no USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select a new country (no USA)
The Country Item should show the selected Country
The State Input should be an input component
You can select new ones and the Menu Item should be updated

Update the menu Country item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
Should send you back and the Country Item should show the selected Country
The State Input should be a Menu Item

Update the menu State item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
The Country Item should show the selected Country
Clicking on the State menu should send you to a new Page
You can select any State
Should send you back to and show you the updated State

QA Steps

Errors on Empty State

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Save
All the Inputs even the Menu Items should show a required field error

Update the menu Country item (no USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select a new country (no USA)
The Country Item should show the selected Country
The State Input should be an input component
You can select new ones and the Menu Item should be updated

Update the menu Country item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
Should send you back and the Country Item should show the selected Country
The State Input should be a Menu Item

Update the menu State item (USA)

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Click on Country item
Should send you to a new page, select United State
The Country Item should show the selected Country
Clicking on the State menu should send you to a new Page
You can select any State
Should send you back to and show you the updated State

Autocomplete address

Sign in using an User that doesn't have address
Go to the Address Page (No inputs should be filled)
Start filling the Address Line 1, a suggestion menu should shows up
Clicking on any address should autocomplete, menu items and inputs

Verify you can update the Menu Items or you can go back to from the selection and the last selection should be in place
Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

pushPage_safari.mp4

pusgPage_chrome.mp4

pushPage_safariError.mp4

Mobile Web - Chrome

pusgPage_chromeMobile.mp4

Mobile Web - Safari

pushPage_safariMobile.mp4

Desktop

pushPage_desktop.mp4

iOS

pushPage_ios.mp4

Android

pushPage_android.mp4

MelvinBot · 2023-05-04T17:50:00Z

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

ArekChr · 2023-05-05T08:38:06Z

src/components/AddressSearch/index.js

@@ -48,6 +48,10 @@ const propTypes = {
    /** A callback function when the value of this field has changed */
    onInputChange: PropTypes.func.isRequired,

+    onCountryChange: PropTypes.func,


Add a comment explaining this function

ArekChr · 2023-05-05T08:38:18Z

src/components/AddressSearch/index.js

@@ -48,6 +48,10 @@ const propTypes = {
    /** A callback function when the value of this field has changed */
    onInputChange: PropTypes.func.isRequired,

+    onCountryChange: PropTypes.func,
+
+    onStateChange: PropTypes.func,


ArekChr · 2023-05-05T09:32:16Z

src/pages/settings/Profile/PersonalDetails/AddressPage.js

+
+                    )}
+                    <FormSpace />
+


Suggested change

ArekChr · 2023-05-05T09:32:25Z

src/pages/settings/Profile/PersonalDetails/AddressPage.js

+                        maxLength={CONST.FORM_CHARACTER_LIMIT}
+                    />
+                    <FormSpace />
+


Suggested change

ArekChr · 2023-05-05T09:32:42Z

src/pages/settings/Profile/PersonalDetails/AddressPage.js

-                    </View>
-                    <View style={styles.mb4}>
+                    ) : (
+


Suggested change

ArekChr · 2023-05-05T10:00:17Z

src/pages/CountrySelectorPage.js

+    return _.filter(countries, country => country.text.toLowerCase().includes(searchValue.toLowerCase()));
+}
+
+function CountrySelectorPage(props) {


DRY, This file has a lot of similarities with the StateSelectorPage component. Could you consider refactoring this code to create more reusable components that can be shared across multiple files?

ArekChr · 2023-05-05T10:01:58Z

src/libs/actions/PersonalDetails.js

@@ -96,6 +96,14 @@ function getCountryISO(countryName) {
    return _.findKey(CONST.ALL_COUNTRIES, country => country === countryName) || '';
 }

+function getCountryNameBy(countryISO) {


Suggested change

function getCountryNameBy(countryISO) {

/**

* Returns the name of the country associated with the provided ISO code.

* If the provided code is invalid, an empty string is returned.

*

* @param {string} countryISO The ISO code of the country to look up.

* @returns {string} The name of the country associated with the provided ISO code.

*/

function getCountryNameBy(countryISO) {

ArekChr · 2023-05-05T10:07:10Z

src/pages/settings/Profile/PersonalDetails/AddressPage.js

@@ -52,23 +52,41 @@ const defaultProps = {
    },
 };

+function FormSpace() {


Consider encapsulating FormSpace in the AddressPage, as it is only used in one component.
Or just use <View style={styles.formSpaceVertical} /> instead.

ArekChr · 2023-05-05T10:12:54Z

src/components/StatePicker.js

+        Navigation.navigate(ROUTES.getUsaStateSelectionRoute(stateTitle || stateISO, Navigation.getActiveRoute()));
+    }, [stateTitle, stateISO]);
+
+    const title = useMemo(() => {


I think memoization is not needed here

I propose to write a separate function for that

If I move it into a function I will need to pass the props, and according to the guidelines I will wrap it into a useCallback so will look kind the same but will recalculate the same values so I would keep it into a memo given that other components can cause re-renders

But this is a string type, which shouldn't cause a re-render in case the title stays the same without using useMemo, that because I requested removing that because it's a cheap function. and it is no matter of the render count

ArekChr · 2023-05-05T10:33:48Z

src/components/StatePicker.js

+    const navigation = props.navigation;
+    const onInputChange = props.onInputChange;
+    const defaultValue = props.defaultValue;
+    const translate = props.translate;


Too many unnecessary declarations here, to improve readability please destruct props in component or call functions with just a simple approach props.navigation.navigate(...)

Yeah, I talked internally about this but seems that we aren't gonna do destruction, but probably we can bring it back in discussion

This topic is mainly about defaultProps with destructing, but let's leave destructing until that is clarified.
I propose to use simple syntax props.navigation.navigate to reduce variable declaration, which will improve the hook's readability right now, it looks a bit messy

ArekChr · 2023-05-08T08:12:29Z

Except for minor improvements that the guidelines are in the process of clarifying. Overall, LGTM 👌

melvin-bot · 2023-05-08T13:55:03Z

@cristipaval @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

gedu · 2023-05-11T05:23:02Z

@best-lucky1030 No, is ready for Expensify review

gedu · 2023-05-16T09:04:08Z

@venture1981 all solved

cristipaval · 2023-05-18T19:17:44Z

bump @mananjadhav

mananjadhav · 2023-05-18T19:21:09Z

Taking longer than expected, reviewed half the files, reviewing the other half and will test it as well.

mananjadhav · 2023-05-22T11:23:15Z

I reviewed and I am fine with the changes, but I see an outstanding comment on making it DRY - StateSelector and CountrySelector. Are we looking to fix this right now ? or follow up in another PR.

cristipaval · 2023-05-22T13:14:01Z

I just thought now that it would be great if @shawnborton has some time to review this from the design standpoint 🙏

gedu · 2023-05-22T14:11:28Z

@mananjadhav that change is done, both components are using OptionsSelectorWithSearch

0xmiros

OptionsSelector already supports search. Why can't we reuse that component?
PronounsPage is a good example. I don't see major UI difference between country/state selector pages and pronouns page.
Also let's implement what pronouns page does:

pre-populate search input and auto-scroll to selected item
deselect support
trim search

bug1.mov

0xmiros · 2023-05-29T13:49:28Z

Hover background area is not familiar to me. Is this design approved? I'd expect to cover full width and enough vertical padding like other menu items.

0xmiros · 2023-05-29T13:54:13Z

I think we should still support shouldSaveDraft in Form.

bug3.mov

0xmiros · 2023-05-29T13:57:29Z

Show state name here, not code.

bug4.mov

shawnborton · 2023-05-29T15:35:08Z

Hover background area is not familiar to me. Is this design approved? I'd expect to cover full width and enough vertical padding like other menu items.

I agree, that does not look great. I agree that the hover styles should match full width like places in the app.

shawnborton · 2023-05-29T15:41:00Z

For these push-to-page rows, are we using a global component? I was surprised to see that the label font size is smaller when there is no value:

For that "emtpy state" of the push row when there is no value set, the font size should be 15px. Can we make sure that is the case for all of these rows, everywhere? cc @cristipaval

Then once a value is set, the label size goes to 13px and it rests above the value, which is at 15px.

shawnborton · 2023-05-29T15:41:57Z

For the list selection views, @cristipaval where do we stand with updating the styles there? we shouldn't have the lines go edge-to-edge, and I thought we had an issue open to make sure the list selection is consistent everywhere.

shawnborton · 2023-05-29T15:42:57Z

When the address dropdown/popover shows, why does that push the rest of the modal content down? I would think it would overlay on top of the content:

shawnborton · 2023-05-29T15:45:50Z

It feels weird to me that state comes before City here:

0xmiros · 2023-05-29T15:45:57Z

When the address dropdown/popover shows, why does that push the rest of the modal content down? I would think it would overlay on top of the content

This is already happening on production. Maybe we can create a separate GH to handle that.

gedu · 2023-06-07T13:37:30Z

@cristipaval @0xmiroslav Conflicts solved

0xmiros · 2023-06-08T13:07:08Z

src/components/OptionsSelectorWithSearch.js

+
+export {greenCheckmark};
+
+export default withLocalize(CountrySelectorPage);


The component name is wrong. Should be aligned with file name.

0xmiros · 2023-06-08T13:08:04Z

src/components/OptionsSelectorWithSearch.js

+
+const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};
+
+function filterOptions(searchValue, countries) {


As this is general component, replace all specific names with general ones. i.e. countries -> data

Suggested change

function filterOptions(searchValue, countries) {

function filterOptions(searchValue, data) {

0xmiros · 2023-06-08T13:10:26Z

@gedu I noticed recent commits are not signed. All commits in the PR should be signed to pass checks.

gedu · 2023-06-09T12:14:57Z

@0xmiroslav I'm having some issues trying to sign previous commits, I tried many commands also some shared into slack but no luck. can I create a new PR? what do you think?

0xmiros · 2023-06-09T12:19:36Z

@gedu we don't recommend force-push once review started but try only this time

gedu · 2023-06-09T12:45:02Z

@gedu we don't recommend force-push once review started but try only this time

@0xmiroslav what do you mean by a force-push? That won't sign the old commits, there are 3 commits that need to be signed, I could sign one of the last ones but those that are from May are getting hard

0xmiros · 2023-06-09T12:46:13Z

Maybe you can push all of work so far in one commit. You're about to do this in new PR, right?

gedu · 2023-06-09T12:50:15Z

Maybe you can push all of work so far in one commit. You're about to do this in new PR, right?

Yeah, I will create a new PR, because one of the unsigned commits is from May, and rebasing is really complicated

0xmiros · 2023-06-09T12:54:38Z

force-push doesn't require rebasing. You can do in this branch as if you do in a new branch
Maybe try this:

git checkout main
git branch -D edu/17548_personal_details_push_to_page (delete local branch)
git checkout -b gedu:edu/17548_personal_details_push_to_page (create new local branch)
commit your changes
git push -f (force push)

0xmiros · 2023-06-09T12:55:51Z

Wanna keep same PR just to have long convo history so far. i.e. important discussions about design

gedu · 2023-06-09T14:51:07Z

@0xmiroslav pushed, sorry for the inconvenience, my main Mac is broken so using my backup Mac I forgot to sign the commits

gedu · 2023-06-13T09:02:25Z

@0xmiroslav any updates?

gedu · 2023-06-15T14:18:44Z

@0xmiroslav is something that I can help with to get this PR to merge?

0xmiros · 2023-06-15T14:32:58Z

Sorry I will review this today. Please pull main.

0xmiros · 2023-06-19T05:24:42Z

OptionsSelector already supports search. Why can't we reuse that component? PronounsPage is a good example. I don't see major UI difference between country/state selector pages and pronouns page. Also let's implement what pronouns page does:

pre-populate search input and auto-scroll to selected item

deselect support

trim search

bug1.mov

This was not addressed yet

0xmiros · 2023-06-19T05:29:09Z

BUG: weird transition animation (the entire page flickers)

bug1.mov

0xmiros · 2023-06-19T05:29:42Z

BUG: after saving state, country is reverted

bug2.mov

0xmiros · 2023-06-19T05:34:38Z

BUG: space between address line 1 and line 2 is bigger than others

0xmiros · 2023-06-19T05:36:24Z

BUG: after selecting address from auto-complete menu,

shows state required error even though it's already filled
error style broken

bug4.mov

0xmiros · 2023-06-19T05:39:17Z

@gedu please compare with staging and fix all differences other than push-to-page features.

gedu · 2023-06-19T10:11:48Z

@0xmiroslav

pre-populate search input and auto-scroll to selected item: FIXED
BUG: weird transition animation (the entire page flickers): New Nav bug, not related with this happens in other screens
BUG: after saving state, country is reverted: This happens in production, I would create another issue to solve this
BUG: space between address line 1 and line 2 is bigger than others: FIXED
BUG: after selecting address from auto-complete menu: FIXED

autocompleted_error.mp4

Navigation bug in another screen

nav_error_split_money.mp4

ArekChr suggested changes May 5, 2023

View reviewed changes

ArekChr mentioned this pull request May 5, 2023

[No QA] Hooks best practices #16882

Merged

55 tasks

gedu marked this pull request as ready for review May 8, 2023 13:54

gedu requested a review from a team as a code owner May 8, 2023 13:54

melvin-bot bot requested review from cristipaval and mananjadhav and removed request for a team May 8, 2023 13:55

cristipaval requested a review from shawnborton May 22, 2023 13:13

cristipaval requested review from 0xmiros and removed request for mananjadhav May 24, 2023 17:30

0xmiros reviewed May 29, 2023

View reviewed changes

0xmiros reviewed Jun 8, 2023

View reviewed changes

gedu force-pushed the edu/17548_personal_details_push_to_page branch from a233ce1 to a13e904 Compare June 9, 2023 10:21

gedu force-pushed the edu/17548_personal_details_push_to_page branch from 8e925bc to 1cfa2ba Compare June 9, 2023 14:43

0xmiros mentioned this pull request Jun 13, 2023

Migrate DetailsPage to functional Component #20449

Closed

57 tasks

gedu force-pushed the edu/17548_personal_details_push_to_page branch from 1c164df to b2e31bd Compare June 15, 2023 15:42

gedu force-pushed the edu/17548_personal_details_push_to_page branch from 64d43f0 to c3f057e Compare June 19, 2023 10:15

mountiny assigned gedu Jun 19, 2023

gedu closed this Jun 19, 2023

gedu force-pushed the edu/17548_personal_details_push_to_page branch from 71f9366 to b997868 Compare June 19, 2023 15:36

gedu mentioned this pull request Jun 19, 2023

17548 Personal details push to page #21049

Merged

58 tasks

-function getCountryNameBy(countryISO) {
+/**
+ * Returns the name of the country associated with the provided ISO code.
+ * If the provided code is invalid, an empty string is returned.
+ *
+ * @param {string} countryISO The ISO code of the country to look up.
+ * @returns {string} The name of the country associated with the provided ISO code.
+ */
+function getCountryNameBy(countryISO) {


		export {greenCheckmark};

		export default withLocalize(CountrySelectorPage);


		const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};

		function filterOptions(searchValue, countries) {

	function filterOptions(searchValue, countries) {
	function filterOptions(searchValue, data) {

17548: Personal details push to page #18424

17548: Personal details push to page #18424

Conversation

gedu commented May 4, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented May 4, 2023

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

ArekChr May 5, 2023 • 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

ArekChr May 8, 2023 • 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

ArekChr commented May 8, 2023 • edited Loading

melvin-bot bot commented May 8, 2023

gedu commented May 11, 2023

gedu commented May 16, 2023

cristipaval commented May 18, 2023

mananjadhav commented May 18, 2023

mananjadhav commented May 22, 2023

cristipaval commented May 22, 2023 • edited Loading

gedu commented May 22, 2023

0xmiros left a comment

Choose a reason for hiding this comment

0xmiros commented May 29, 2023

0xmiros commented May 29, 2023

0xmiros commented May 29, 2023

shawnborton commented May 29, 2023

shawnborton commented May 29, 2023

shawnborton commented May 29, 2023

shawnborton commented May 29, 2023

shawnborton commented May 29, 2023

0xmiros commented May 29, 2023

gedu commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmiros commented Jun 8, 2023 • edited Loading

gedu commented Jun 9, 2023

0xmiros commented Jun 9, 2023

gedu commented Jun 9, 2023

0xmiros commented Jun 9, 2023

gedu commented Jun 9, 2023

0xmiros commented Jun 9, 2023

0xmiros commented Jun 9, 2023

gedu commented Jun 9, 2023

gedu commented Jun 13, 2023

gedu commented Jun 15, 2023

0xmiros commented Jun 15, 2023

0xmiros commented Jun 19, 2023

0xmiros commented Jun 19, 2023

0xmiros commented Jun 19, 2023

0xmiros commented Jun 19, 2023

0xmiros commented Jun 19, 2023

0xmiros commented Jun 19, 2023

gedu commented Jun 19, 2023

gedu commented May 4, 2023 •

edited

Loading

ArekChr May 5, 2023 •

edited

Loading

ArekChr May 8, 2023 •

edited

Loading

ArekChr commented May 8, 2023 •

edited

Loading

cristipaval commented May 22, 2023 •

edited

Loading

0xmiros commented Jun 8, 2023 •

edited

Loading