Refactor NewContactMethodPage to use hooks #17472
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||
| import React, { | ||||||
| useCallback, useMemo, useRef, useState, | ||||||
| } from 'react'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
| import {View} from 'react-native'; | ||||||
| import {ScrollView} from 'react-native-gesture-handler'; | ||||||
|
|
@@ -52,102 +54,87 @@ const defaultProps = { | |||||
| loginList: {}, | ||||||
| }; | ||||||
|
|
||||||
| function NewContactMethodPage(props) { | ||||||
|
Contributor
There was a problem hiding this comment. 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
Contributor
Author
There was a problem hiding this comment. 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. |
||||||
| const [login, setLogin] = useState(''); | ||||||
| const [password, setPassword] = useState(''); | ||||||
| const loginInputRef = useRef(null); | ||||||
|
|
||||||
| const handleLoginChange = useCallback((value) => { | ||||||
| setLogin(value.trim()); | ||||||
| }, []); | ||||||
|
|
||||||
| const handlePasswordChange = useCallback((value) => { | ||||||
| setPassword(value.trim()); | ||||||
| }, []); | ||||||
|
|
||||||
| const isFormValid = useMemo(() => { | ||||||
| const phoneLogin = LoginUtils.getPhoneNumberWithoutSpecialChars(login); | ||||||
|
|
||||||
| return (Permissions.canUsePasswordlessLogins(props.betas) || password) | ||||||
| && (Str.isValidEmail(login) || Str.isValidPhone(phoneLogin)); | ||||||
| }, [login, password, props.betas]); | ||||||
|
|
||||||
| const submitForm = useCallback(() => { | ||||||
|
Contributor
There was a problem hiding this comment. Why do we need a
Contributor
Author
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. That makes sense, thanks
Contributor
Author
There was a problem hiding this comment. 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 |
||||||
| // If this login already exists, just go back. | ||||||
| if (lodashGet(props.loginList, login)) { | ||||||
| Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS); | ||||||
| return; | ||||||
| } | ||||||
| User.addNewContactMethodAndNavigate(login, password); | ||||||
| }, [login, props.loginList, password]); | ||||||
|
|
||||||
| return ( | ||||||
| <ScreenWrapper | ||||||
| onTransitionEnd={() => { | ||||||
| if (!loginInputRef.current) { | ||||||
| return; | ||||||
| } | ||||||
| loginInputRef.current.focus(); | ||||||
| }} | ||||||
| > | ||||||
| <HeaderWithCloseButton | ||||||
| title={props.translate('contacts.newContactMethod')} | ||||||
| shouldShowBackButton | ||||||
| onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS)} | ||||||
| onCloseButtonPress={() => Navigation.dismissModal(true)} | ||||||
| /> | ||||||
| <ScrollView> | ||||||
| <Text style={[styles.ph5, styles.mb5]}> | ||||||
| {props.translate('common.pleaseEnterEmailOrPhoneNumber')} | ||||||
| </Text> | ||||||
| <View style={[styles.ph5, styles.mb6]}> | ||||||
| <TextInput | ||||||
| label={`${props.translate('common.email')}/${props.translate('common.phoneNumber')}`} | ||||||
| ref={loginInputRef} | ||||||
| value={login} | ||||||
| onChangeText={handleLoginChange} | ||||||
| autoCapitalize="none" | ||||||
| returnKeyType={Permissions.canUsePasswordlessLogins(props.betas) ? 'done' : 'next'} | ||||||
| /> | ||||||
| </View> | ||||||
| {!Permissions.canUsePasswordlessLogins(props.betas) | ||||||
| && ( | ||||||
| <View style={[styles.ph5, styles.mb6]}> | ||||||
| <TextInput | ||||||
| label={props.translate('common.password')} | ||||||
| value={password} | ||||||
| onChangeText={handlePasswordChange} | ||||||
| returnKeyType="done" | ||||||
| /> | ||||||
| </View> | ||||||
| )} | ||||||
| </ScrollView> | ||||||
| <FixedFooter style={[styles.flexGrow0]}> | ||||||
| <Button | ||||||
| success | ||||||
| isDisabled={!isFormValid} | ||||||
| text={props.translate('common.add')} | ||||||
| onPress={submitForm} | ||||||
| pressOnEnter | ||||||
| /> | ||||||
| </FixedFooter> | ||||||
| </ScreenWrapper> | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| NewContactMethodPage.propTypes = propTypes; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took this from here #17472 (comment) thanks @eVoloshchak !
And it got rid of the console warning I was getting. I tested that it didn't break the sign-in page either which happened the last time I tried to solve this so think it's working 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On IOS I get this error :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this should be fixed by the change to
Proptypes.object, let me see if I can get ios working locally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been resolved?