Refactor NewContactMethodPage to use hooks #17472

bondydaa · 2023-04-14T23:23:27Z

Details

Refactoring the NewContactMethodPage component to a functional component and to use hooks.

Fixed Issues

$ #16291

Tests And QA Steps

Sign in and go to Profile -> Contact Method
Add new contact method which isn't claimed by another user
Verify that you are redirected to Contact Methods and the newly added contact method is in the list
Validate it and verify that the green indicator goes away
Add a new contact method which is added already to another account
Verify that you are redirected to the contact methods list and the newly added one has a red indicator with an error
Verify that you can successfully remove the contact methods added in the steps above

Verify that no errors appear in the JS console

Offline tests

Go to Contact Methods on deviceA
Sign in with the same account and go to contact methods on deviceB
Turn off the internet on deviceA
Add a new contact method
Verify that you are redirected back to Contact Methods and the newly added contact method is in a pending action
Turn on the internet on deviceA
Verify that the newly added contact method is not greyed out anymore and you can click and validate it
Verify on deviceB that the new contact method is listed in Contact Methods page

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

16291_web.mp4

Mobile Web - Chrome

Mobile Web - Safari

Desktop

2023-04-14_17-10-11.mp4

iOS

Android

…onent-migration

MelvinBot · 2023-04-14T23:24:02Z

@eVoloshchak @Beamanator 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]

bondydaa · 2023-04-14T23:25:48Z

I couldn't get ios or android to build locally, probably b/c i was doing old dot dev recently so if the c+ could get those screenshots for me that'd be awesome. otherwise I'll take a look next week.

Another thing is that I am seeing this console error

Which I tried to fix with this: 64c0f0f

but that led to a bunch of other errors in the app, like on the login form so I think we might need to ignore it until we refactor that in a single go. But let me know if there's something else I should try/do.

eVoloshchak · 2023-04-17T09:00:48Z

Attaching the screen recordings for iOS and Android

iOS

Screen.Recording.2023-04-17.at.11.44.57.mov

Android

Screen_Recording_20230417-114306_New.Expensify.mp4

As for the Proptypes error with innerRef, we could change this line to

    innerRef: PropTypes.oneOfType([
        PropTypes.func,
        PropTypes.shape({current: PropTypes.instanceOf(Element)}),
    ]),

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

eVoloshchak · 2023-04-17T08:57:50Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js


-    submitForm() {
+    const submitForm = useCallback(() => {


Why do we need a useCallback here?

It probably isn't strictly required here per the react docs https://react.dev/reference/react/useCallback#usage it suggests you probably only need it for a performance optimization.

But I chatted with @marcaaron a bit 1:1 about it, sounds like there was a discussion somewhere (not sure where probably open-source but I haven't looked for it yet) where we landed on preferring to just always use useCallback

the resolution was that we should just always use it since the performance downsides of missing when we should use it vs not can be pretty bad.
⁦Or said another way - we'll probably mess stuff up more by not using it then we will by using it

That makes sense, thanks

hmm interesting thread that seems to be saying the opposite of what we decided here https://expensify.slack.com/archives/C01GTK53T8Q/p1680096510744619

just wanted to share so others didn't keep thinking we should prefer useCallback by default.

Co-authored-by: Eugene Voloshchak <copyreading@gmail.com>

eVoloshchak · 2023-04-17T15:56:31Z

@bondydaa , is this ready for testing? What are your thoughts on this?

marcaaron

Looks super great - nailed pretty much everything 👍👍

marcaaron · 2023-04-17T19:29:31Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

-    validateForm() {
-        const login = this.state.login.trim();
-        const phoneLogin = LoginUtils.getPhoneNumberWithoutSpecialChars(login);
+    const validateForm = useCallback(() => {


NAB, since we are calling this validateForm() method in the render... I'd maybe update this to be a useMemo() and return the value instead of a function...

const isFormValid = useMemo(() => {...}, dependencies);

It's kind of minor, but I think slightly better in that we don't need to call the function on render and can just use the value directly instead.

yeah i was really confused looking at this and was thinking it'd be better if it was a single value not a function but then didn't want to accidentally break something. can update though.

bondydaa · 2023-04-17T19:59:40Z

I def don't know enough to feel comfortable signing off on this change #17472 (comment)

@marcaaron what are your thoughts? Would that allow us to use both the current code that exists everywhere and then use the useRef hook here? then once we've converted everything we can change it to only be compatible with useRef?

marcaaron · 2023-04-17T21:59:13Z

Would that allow us to use both the current code that exists everywhere and then use the useRef hook here? then once we've converted everything we can change it to only be compatible with useRef?

I think the suggestion is fine. Ultimately, it's not guaranteed that all refs would have the {current: any} type. Some could still be callbacks (function) as callback refs are still a thing (just maybe not as common as we use them today).

fedirjh · 2023-04-17T22:00:02Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

+                        label={`${props.translate('common.email')}/${props.translate('common.phoneNumber')}`}
+                        ref={loginInputRef}
+                        value={login}
+                        onChangeText={value => setLogin(value)}


Suggested change

onChangeText={value => setLogin(value)}

onChangeText={setLogin}

Instead of defining a new arrow function, I believe we can simply pass setLogin here. This would result in a minor performance boost, as we would not need to recreate the function each time the component is updated.

oh cool thanks, makes sense. I don't think I realized the value would be passed by default to the function here.

fedirjh · 2023-04-17T22:00:07Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

+                            <TextInput
+                                label={props.translate('common.password')}
+                                value={password}
+                                onChangeText={value => setPassword(value)}


Suggested change

onChangeText={value => setPassword(value)}

onChangeText={setPassword}

Same here.

…rmance hits

eVoloshchak

Tests well

bondydaa · 2023-04-24T21:05:00Z

@marcaaron @Beamanator @cristipaval can you give it another look please before more conflicts pop up 🙏

Beamanator

Looks great! 1 tiny question, approving since it's very small

Beamanator · 2023-04-25T10:10:03Z

src/components/TextInput/baseTextInputPropTypes.js

@@ -47,7 +47,10 @@ const propTypes = {
    hideFocusedState: PropTypes.bool,

    /** Forward the inner ref */
-    innerRef: PropTypes.func,
+    innerRef: PropTypes.oneOfType([


Has this been resolved?

Beamanator · 2023-04-25T10:14:18Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

-class NewContactMethodPage extends Component {
-    constructor(props) {
-        super(props);
+function NewContactMethodPage(props) {


Suuuper just wondering, why aren't we doing this? I see some components doing this & some doing what you have, I feel like it would be nice to standardize but maybe it doesn't matter for now

Suggested change

function NewContactMethodPage(props) {

const NewContactMethodPage = (props) => {

The difference between these 2 things is that defining it as a function means that it will be hoisted into the global scope. Defining it as a variable prevents this hoisting. See for the nitty gritty on why is may or may not matter https://github.com/getify/You-Dont-Know-JS/blob/2nd-ed/scope-closures/ch5.md#hoisting-declaration-vs-expression

But I agree, right now we are fine with either and we should probably standardize on one or the other. I don't have a strong preference for either format.

bondydaa · 2023-04-25T16:11:27Z

:marshmellow-duet: conflicts again, fixing them.

…o the state

bondydaa · 2023-04-25T17:21:51Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

+    const handleLoginChange = useCallback((value) => {
+        setLogin(value.trim());
+    }, [login]);
+
+    const handlePasswordChange = useCallback((value) => {
+        setPassword(value.trim());
+    }, [password]);


I did this to fix the merge conflict caused by #17817 and think it's a better solution so that the values in state never have leading or trailing whitespace.

It does add one small UX quirk that I'm not sure if we care about: https://user-images.githubusercontent.com/4073354/234353895-d83bf242-44b4-48fc-8032-f85a0323c50b.mp4

basically if you go back to the start of the text and hit a space b/c we .trim the value the cursor jumps to the end of the text.

Personally I don't really think this is a problem and probably only something QA will do while testing - real users most likely are not attempting this and will require a bit more code to prevent it but open to other opinions.

bondydaa · 2023-04-25T17:24:01Z

updated

fedirjh · 2023-04-25T17:51:33Z

src/pages/settings/Profile/Contacts/NewContactMethodPage.js

+    const handleLoginChange = value => setLogin(value.trim());
+    const handlePasswordChange = value => setPassword(value.trim());


@bondydaa We can use useCallback and we pass empty dependency array

const handlePasswordChange = useCallback((value) => { setPassword(value.trim()); }, []);

ah that is probably what I was missing! thanks for the tip.

bondydaa · 2023-04-25T20:46:34Z

(dj khaled voice) and another 1.

OSBotify · 2023-04-25T21:20:25Z

✋ 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 · 2023-04-26T06:34:16Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.6-0 🚀

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

OSBotify · 2023-04-27T04:49:19Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.6-0 🚀

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

bondydaa added 6 commits April 14, 2023 16:50

import hook methods and create new state/ref variables

1ea1f7a

port methods to new format

2cd76d1

update remaining component body to work with functional component

a7768f0

i think fix error related to ref being an object not a function

64c0f0f

Merge branch 'main' of github.com:Expensify/App into bondy-login-comp…

f14b76c

…onent-migration

undo because it breaks everything

b55028d

bondydaa requested review from cristipaval and marcaaron April 14, 2023 23:23

bondydaa self-assigned this Apr 14, 2023

bondydaa requested a review from a team as a code owner April 14, 2023 23:23

melvin-bot bot requested review from Beamanator and eVoloshchak and removed request for a team April 14, 2023 23:24

bondydaa mentioned this pull request Apr 14, 2023

[HOLD for payment 2023-05-04] [$250] Migrate NewContactMethodPage.js to function component #16291

Closed

1 task

eVoloshchak suggested changes Apr 17, 2023

View reviewed changes

bondydaa and others added 2 commits April 17, 2023 09:23

remove "this" left over from class implementation

c79c808

Co-authored-by: Eugene Voloshchak <copyreading@gmail.com>

use different name b/c password is already defined

da65870

Co-authored-by: Eugene Voloshchak <copyreading@gmail.com>

marcaaron previously approved these changes Apr 17, 2023

View reviewed changes

fedirjh reviewed Apr 17, 2023

View reviewed changes

bondydaa added 3 commits April 17, 2023 16:12

no need for arrow functions can just call the state set methods directly

615c441

convert from method to boolean value and add useMemo to prevent perfo…

ce4ba20

…rmance hits

support both hook style refs and class style hooks

80b2d92

bondydaa dismissed marcaaron’s stale review via 80b2d92 April 17, 2023 22:28

eVoloshchak previously approved these changes Apr 21, 2023

View reviewed changes

marcaaron previously approved these changes Apr 24, 2023

View reviewed changes

Beamanator previously approved these changes Apr 25, 2023

View reviewed changes

bondydaa added 2 commits April 25, 2023 10:27

fixing conflicts

5bcd178

create handlers so we can always trim the values before adding them t…

8adab08

…o the state

bondydaa dismissed stale reviews from Beamanator, marcaaron, eVoloshchak, and fedirjh via 8adab08 April 25, 2023 17:15

bondydaa commented Apr 25, 2023

View reviewed changes

bondydaa requested review from cristipaval, fedirjh, marcaaron, eVoloshchak and Beamanator April 25, 2023 17:22

remove unnecessary dependency

3380130

cant use useCallback since we dont have a dependency to pass it

a061347

fedirjh reviewed Apr 25, 2023

View reviewed changes

use useCallback with empty dependency array to avoid performance woes

91e49c9

marcaaron approved these changes Apr 25, 2023

View reviewed changes

marcaaron merged commit b047ec7 into main Apr 25, 2023

marcaaron deleted the bondy-login-component-migration branch April 25, 2023 21:12

OSBotify mentioned this pull request Apr 26, 2023

Deploy Checklist: New Expensify 2023-04-26 #18017

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor NewContactMethodPage to use hooks #17472

Refactor NewContactMethodPage to use hooks #17472

bondydaa commented Apr 14, 2023

MelvinBot commented Apr 14, 2023

bondydaa commented Apr 14, 2023

eVoloshchak commented Apr 17, 2023

eVoloshchak Apr 17, 2023

bondydaa Apr 17, 2023

eVoloshchak Apr 17, 2023

bondydaa May 19, 2023

eVoloshchak commented Apr 17, 2023 •

edited

Loading

marcaaron left a comment

marcaaron Apr 17, 2023

bondydaa Apr 17, 2023

bondydaa commented Apr 17, 2023 •

edited

Loading

marcaaron commented Apr 17, 2023

fedirjh Apr 17, 2023

bondydaa Apr 17, 2023

fedirjh Apr 17, 2023

eVoloshchak left a comment

bondydaa commented Apr 24, 2023

Beamanator left a comment •

edited

Loading

Beamanator Apr 25, 2023

Beamanator Apr 25, 2023

bondydaa Apr 25, 2023

bondydaa commented Apr 25, 2023

bondydaa Apr 25, 2023

bondydaa commented Apr 25, 2023

fedirjh Apr 25, 2023

bondydaa Apr 25, 2023

bondydaa commented Apr 25, 2023

OSBotify commented Apr 25, 2023

OSBotify commented Apr 26, 2023

OSBotify commented Apr 27, 2023

	onChangeText={value => setLogin(value)}
	onChangeText={setLogin}

	onChangeText={value => setPassword(value)}
	onChangeText={setPassword}

	function NewContactMethodPage(props) {
	const NewContactMethodPage = (props) => {

		const handleLoginChange = value => setLogin(value.trim());
		const handlePasswordChange = value => setPassword(value.trim());

Refactor NewContactMethodPage to use hooks #17472

Refactor NewContactMethodPage to use hooks #17472

Conversation

bondydaa commented Apr 14, 2023

Details

Fixed Issues

Tests And QA Steps

Offline tests

PR Author Checklist

Screenshots/Videos

MelvinBot commented Apr 14, 2023

bondydaa commented Apr 14, 2023

eVoloshchak commented Apr 17, 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

eVoloshchak commented Apr 17, 2023 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bondydaa commented Apr 17, 2023 • edited Loading

marcaaron commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eVoloshchak left a comment

Choose a reason for hiding this comment

bondydaa commented Apr 24, 2023

Beamanator 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

bondydaa commented Apr 25, 2023

Choose a reason for hiding this comment

bondydaa commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bondydaa commented Apr 25, 2023

OSBotify commented Apr 25, 2023

OSBotify commented Apr 26, 2023

OSBotify commented Apr 27, 2023

eVoloshchak commented Apr 17, 2023 •

edited

Loading

bondydaa commented Apr 17, 2023 •

edited

Loading

Beamanator left a comment •

edited

Loading