-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TS migration] Migrate SettingsProfileContacts
page to TypeScript
#35305
Conversation
src/components/MagicCodeInput.tsx
Outdated
@@ -427,4 +427,5 @@ function MagicCodeInput( | |||
|
|||
MagicCodeInput.displayName = 'MagicCodeInput'; | |||
|
|||
export type {MagicCodeInputHandle}; |
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.
NAB: I would move it below export default ...
to match other files
|
||
type ContactMethodDetailsPageProps = ContactMethodDetailsPageOnyxProps & StackScreenProps<SettingsNavigatorParamList, typeof SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_DETAILS>; | ||
|
||
function ContactMethodDetailsPage({loginList = {}, session = {}, myDomainSecurityGroups = {}, securityGroups, isLoadingReportData = true, route}: ContactMethodDetailsPageProps) { |
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.
function ContactMethodDetailsPage({loginList = {}, session = {}, myDomainSecurityGroups = {}, securityGroups, isLoadingReportData = true, route}: ContactMethodDetailsPageProps) { | |
function ContactMethodDetailsPage({loginList, session, myDomainSecurityGroups, securityGroups, isLoadingReportData = true, route}: ContactMethodDetailsPageProps) { |
since we are using optional chaining its not needed to add default values
/** | ||
* Gets the current contact method from the route params | ||
*/ | ||
const contactMethod = useMemo(() => { |
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.
Please add return type to this const
|
||
function ContactMethodsPage(props) { | ||
function ContactMethodsPage({loginList = {}, session, route}: ContactMethodsPageProps) { |
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.
Same here , no need to add default value to loginList
const phoneLogin = LoginUtils.getPhoneLogin(values.phoneOrEmail); | ||
const validateIfnumber = LoginUtils.validateNumber(phoneLogin); | ||
const submitDetail = (validateIfnumber || values.phoneOrEmail).trim().toLowerCase(); | ||
|
||
User.addNewContactMethodAndNavigate(submitDetail); | ||
}; | ||
|
||
function NewContactMethodPage(props) { | ||
function NewContactMethodPage({loginList = {}, route}: NewContactMethodPageProps) { |
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.
Same here
style={[styles.flexGrow1, styles.mh5]} | ||
enabledWhenOffline | ||
> | ||
<Text style={[styles.mb5]}>{props.translate('common.pleaseEnterEmailOrPhoneNumber')}</Text> | ||
<Text style={[styles.mb5]}>{translate('common.pleaseEnterEmailOrPhoneNumber')}</Text> |
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.
<Text style={[styles.mb5]}>{translate('common.pleaseEnterEmailOrPhoneNumber')}</Text> | |
<Text style={styles.mb5}>{translate('common.pleaseEnterEmailOrPhoneNumber')}</Text> |
pendingFields: PropTypes.objectOf(PropTypes.objectOf(PropTypes.string)), | ||
}).isRequired, | ||
/** Specifies autocomplete hints for the system, so it can provide autofill */ | ||
autoComplete?: 'sms-otp' | 'one-time-code'; |
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.
maybe worth creating a seperate type for that?
const validateLoginError = ErrorUtils.getEarliestErrorField(loginData, 'validateLogin'); | ||
const shouldDisableResendValidateCode = props.network.isOffline || props.account.isLoading; | ||
const focusTimeoutRef = useRef(null); | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing |
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.
please add explanation why this was added
indicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO; | ||
} | ||
|
||
// Default to using login key if we deleted login.partnerUserID optimistically | ||
// but still need to show the pending login being deleted while offline. | ||
const partnerUserID = login.partnerUserID || loginName; | ||
const menuItemTitle = Str.isSMSLogin(partnerUserID) ? props.formatPhoneNumber(partnerUserID) : partnerUserID; | ||
const partnerUserID = login?.partnerUserID ?? loginName; |
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.
const partnerUserID = login?.partnerUserID ?? loginName; | |
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
const partnerUserID = login?.partnerUserID || loginName; |
partnerUserID
is updated to empty string when the contact is deleted optimistically
if (!login.partnerUserID && _.isEmpty(pendingAction)) { | ||
const loginMenuItems = sortedLoginNames.map((loginName) => { | ||
const login = loginList?.[loginName]; | ||
const pendingAction = login?.pendingFields?.deletedLogin ?? login?.pendingFields?.addedLogin; |
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.
const pendingAction = login?.pendingFields?.deletedLogin ?? login?.pendingFields?.addedLogin; | |
const pendingAction = login?.pendingFields?.deletedLogin ?? login?.pendingFields?.addedLogin ?? undefined; |
Let's fallback to undefined
const loginMenuItems = sortedLoginNames.map((loginName) => { | ||
const login = loginList?.[loginName]; | ||
const pendingAction = login?.pendingFields?.deletedLogin ?? login?.pendingFields?.addedLogin; | ||
if (!login?.partnerUserID && isEmptyObject(pendingAction)) { |
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.
if (!login?.partnerUserID && isEmptyObject(pendingAction)) { | |
if (!login?.partnerUserID && !pendingAction) { |
pendingAction
is not an object.
|
||
return ( | ||
<OfflineWithFeedback | ||
pendingAction={pendingAction} | ||
pendingAction={pendingAction ?? undefined} |
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.
pendingAction={pendingAction ?? undefined} | |
pendingAction={pendingAction} |
@@ -105,7 +82,7 @@ function ContactMethodsPage(props) { | |||
brickRoadIndicator={indicator} | |||
shouldShowBasicTitle | |||
shouldShowRightIcon | |||
disabled={!_.isEmpty(pendingAction)} | |||
disabled={!isEmptyObject(pendingAction)} |
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.
disabled={!isEmptyObject(pendingAction)} | |
disabled={!!pendingAction} |
name="validateCode" | ||
value={validateCode} | ||
onChangeText={onTextInput} | ||
errorText={formError.validateCode ? props.translate(formError.validateCode) : ErrorUtils.getLatestErrorMessage(props.account)} | ||
hasError={!_.isEmpty(validateLoginError)} | ||
errorText={formError?.validateCode ? translate(formError?.validateCode as TranslationPaths) : ErrorUtils.getLatestErrorMessage(account ?? {})} |
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.
Let's avoid type casting: Let's create a new type for the formError
type ValidateCodeFormError = {
validateCode?: TranslationPaths;
};
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
const shouldDisableResendValidateCode = isOffline || account?.isLoading; |
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.
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
const shouldDisableResendValidateCode = isOffline || account?.isLoading; | |
const shouldDisableResendValidateCode = !!isOffline || account?.isLoading; |
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.
Why do we need this? isOffline
is a boolen.
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.
It could be null
as well.
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.
We can't use nullish coalescing here because it will fail when isOffline
is false
.
|
||
// Replacing spaces with "hard spaces" to prevent breaking the number | ||
const formattedContactMethod = Str.isSMSLogin(contactMethod) ? formatPhoneNumber(contactMethod).replace(/ /g, '\u00A0') : contactMethod; | ||
const hasMagicCodeBeenSent = loginList?.[contactMethod].validateCodeSent; |
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.
const hasMagicCodeBeenSent = loginList?.[contactMethod].validateCodeSent; | |
const hasMagicCodeBeenSent = !!loginData.validateCodeSent; |
|
||
<ValidateCodeForm | ||
contactMethod={contactMethod} | ||
hasMagicCodeBeenSent={!!hasMagicCodeBeenSent} |
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.
hasMagicCodeBeenSent={!!hasMagicCodeBeenSent} | |
hasMagicCodeBeenSent={hasMagicCodeBeenSent} |
messages={{0: 'resendValidationForm.linkHasBeenResent'}} | ||
/> | ||
)} | ||
</View> | ||
</OfflineWithFeedback> | ||
<OfflineWithFeedback | ||
pendingAction={lodashGet(loginData, 'pendingFields.validateLogin', null)} | ||
pendingAction={loginData.pendingFields?.validateLogin ?? undefined} |
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.
Maybe we have to update PendingAction
to accept null
values.
Resolved all comments. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeCleanShot.2024-02-19.at.02.43.06.mp4Android: mWeb ChromeCleanShot.2024-02-19.at.02.40.43.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-19.at.02.22.52.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-19.at.02.29.46.mp4MacOS: Chrome / SafariCleanShot.2024-02-19.at.02.12.10.mp4MacOS: DesktopCleanShot.2024-02-19.at.02.54.24.mp4 |
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.
Looks good to me
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25218 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@fedirjh @youssef-lr I did not see any internal engineer assigned here for approval. |
@kubabutkiewicz would you like to give this a final review? |
Will do that today! |
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.
lgtm!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.4.44-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
Details
Migrate
SettingsProfileContacts
page to TypeScriptFixed Issues
$ #25218
PROPOSAL:
Tests
Offline tests
NA
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-02-05.at.21.58.22-compressed.mov
Screen.Recording.2024-02-05.at.21.25.57-compressed.mov
Screen.Recording.2024-02-05.at.21.25.14-compressed.mov
Screen.Recording.2024-02-05.at.17.21.25-compressed.mov
MacOS: Desktop
Screen.Recording.2024-02-05.at.21.58.22-compressed.mov
Screen.Recording.2024-02-05.at.21.25.57-compressed.mov
Screen.Recording.2024-02-05.at.21.25.14-compressed.mov
Screen.Recording.2024-02-05.at.17.21.25-compressed.mov