-
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
Refactor/31312 consolidate getdisplayname methods pt2 #33930
Changes from 4 commits
2a73e48
1c53bfa
45b5e35
250dc18
d8b9af7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,20 +43,19 @@ Onyx.connect({ | |
}); | ||
|
||
/** | ||
* Returns the displayName for a user | ||
* Creates a new displayName for a user based on passed personal details or login. | ||
*/ | ||
function getDisplayName(login: string, personalDetail: Pick<PersonalDetails, 'firstName' | 'lastName'> | null): string { | ||
function createDisplayName(login: string, personalDetails: Pick<PersonalDetails, 'firstName' | 'lastName'>): string { | ||
// If we have a number like [email protected] then let's remove @expensify.sms and format it | ||
// so that the option looks cleaner in our UI. | ||
const userLogin = LocalePhoneNumber.formatPhoneNumber(login); | ||
const userDetails = personalDetail ?? allPersonalDetails?.[login]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. - ?? allPersonalDetails?.[login]; For 2nd case, this is major change. Others are just code refactor, not affecting logic, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @situchan could you explain what do you mean by major change? We're always passing personalDetails object that should not be empty, so I removed this check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic change I mean. Others are just variable re-naming, etc |
||
|
||
if (!userDetails) { | ||
if (!personalDetails) { | ||
return userLogin; | ||
} | ||
|
||
const firstName = userDetails.firstName ?? ''; | ||
const lastName = userDetails.lastName ?? ''; | ||
const firstName = personalDetails.firstName ?? ''; | ||
const lastName = personalDetails.lastName ?? ''; | ||
const fullName = `${firstName} ${lastName}`.trim(); | ||
|
||
// It's possible for fullName to be empty string, so we must use "||" to fallback to userLogin. | ||
|
@@ -150,7 +149,7 @@ function updateDisplayName(firstName: string, lastName: string) { | |
[currentUserAccountID]: { | ||
firstName, | ||
lastName, | ||
displayName: getDisplayName(currentUserEmail ?? '', { | ||
displayName: createDisplayName(currentUserEmail ?? '', { | ||
firstName, | ||
lastName, | ||
}), | ||
|
@@ -566,7 +565,7 @@ export { | |
deleteAvatar, | ||
extractFirstAndLastNameFromAvailableDetails, | ||
getCountryISO, | ||
getDisplayName, | ||
createDisplayName, | ||
getPrivatePersonalDetails, | ||
openPersonalDetailsPage, | ||
openPublicProfilePage, | ||
|
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.
Just curious why we are changing the name. "create" implies to me that we're going to store this somewhere.
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.
@puneetlath I've changed the name bc we're not getting it from Onyx but creating it from given firstName and lastName and then send to API and save in Onyx as optimisticData
I also wanted to differentiate from other methods that are just getting the name from personailDetails from Onyx
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.
Ahhh I see, ok that makes sense.