Prioritize display name to display in tooltip text #34213

dukenv0307 · 2024-01-10T07:49:49Z

Details

Prioritize display name to display in tooltip text

Fixed Issues

$ #33740
PROPOSAL: #33740 (comment)

Tests

Create a group chat with at least 5 users with custom display name
Create a split bill with all participants in the group
Hover the counter in the split bill preview
Verify that the display name of these users are displayed
Go to workspace > Invite
Choose some recent contact and enter some new users after this. Make sure the total invited users are more than 8
Go to the next page
Hover the count number and verify that the tooltip text is displayed correctly for new users (display the email or phone number)
Hover on the user with the custom display name in LHN and verify that the display name is displayed

Verify that no errors appear in the JS console

Offline tests

Pre-condition: Create a group chat with at least 5 users with the custom display name in online

Create a split bill with all participants in the group
Hover the counter in the split bill preview
Verify that the display name of these users are displayed

QA Steps

Create a group chat with at least 5 users with custom display name
Create a split bill with all participants in the group
Hover the counter in the split bill preview
Verify that the display name of these users are displayed
Go to workspace > Invite
Choose some recent contact and enter some new users after this. Make sure the total invited users are more than 8
Go to the next page
Hover the count number and verify that the tooltip text is displayed correctly for new users (display the email or phone number)
Hover on the user with the custom display name in LHN and verify that the display name is displayed

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-01-10.at.14.36.17.mov

Android: mWeb Chrome

Screen.Recording.2024-01-10.at.14.33.23.mov

iOS: Native

Screen.Recording.2024-01-10.at.14.38.45.mov

iOS: mWeb Safari

Screen.Recording.2024-01-10.at.14.31.55.mov

MacOS: Chrome / Safari

Screen.Recording.2024-01-10.at.14.40.03.mov

MacOS: Desktop

Screen.Recording.2024-01-10.at.14.44.34.mov

melvin-bot · 2024-01-10T07:50:05Z

@parasharrajat Please 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]

parasharrajat · 2024-01-16T11:27:13Z

@dukenv0307 Please resolve conflicts.

dukenv0307 · 2024-01-16T11:30:21Z

@parasharrajat I resolved.

parasharrajat · 2024-01-16T14:22:45Z

I noticed that the tooltip on the header level is not showing all the users. I request money with 6 users and the tooltip is only showing 3.

Screen.Recording.2024-01-16.at.7.51.52.PM.mov

dukenv0307 · 2024-01-18T10:02:04Z

@parasharrajat I fixed this case above.

parasharrajat · 2024-01-22T11:52:30Z

It is still the same for me. This is also present on staging so not related to this PR but can be fixed together as we are fixing the same functionality. If the problem looks more involved, let me know.

Screen.Recording.2024-01-22.at.5.22.37.PM.mov

parasharrajat · 2024-01-22T12:03:34Z

@dukenv0307 Please merge the main as well. Thanks.

parasharrajat · 2024-01-22T12:04:50Z

src/components/MultipleAvatars.tsx

@@ -106,7 +107,6 @@ function MultipleAvatars({
    let avatarContainerStyles = StyleUtils.getContainerStyles(size, isInReportAction);
    const {singleAvatarStyle, secondAvatarStyles} = useMemo(() => avatarSizeToStylesMap[size as AvatarSizeToStyles] ?? avatarSizeToStylesMap.default, [size, avatarSizeToStylesMap]);

-    const tooltipTexts = useMemo(() => (shouldShowTooltip ? icons.map((icon) => icon.name) : ['']), [shouldShowTooltip, icons]);


Why do we have to remove this memo. Why can't we just add the ReportUtils.getUserDetailsTooltipText( part to it.

@parasharrajat Actually, we use tooltipTexts as an array of user tooltip text and it will be slice accordingly below. We have two option

No need to change any thing in MultipleAvatars because icon.name already uses getDisplayNameForParticipant function

keep the change

@parasharrajat I merged main, what do you think about my comment above?

What I meant to say was that tooltiptexts contain all the icons and it is used twice in the code below. Why can't we calculate the getUserDetailsTooltipText once over this array here and use to where needed.

Now, you are calculating the getUserDetailsTooltipText from these icons at both places.

Both those places use different slicing over this array.

My suggestion was that we could convert the tooltipTexts icon array to an array of getUserDetailsTooltipText. Then just use this in those slicing operations.

@parasharrajat was your concern here addressed/resolved?

It is no blocker for me.

This is now addressed.

parasharrajat · 2024-01-29T11:09:37Z

@dukenv0307 Any thoughts?

dukenv0307 · 2024-01-29T11:24:52Z

I will update tomorrow.

parasharrajat · 2024-01-29T11:25:35Z

Works for me.

parasharrajat · 2024-02-05T15:26:53Z

@dukenv0307 Please merge main before make any changes.

parasharrajat · 2024-02-19T07:12:37Z

@dukenv0307 Can you please merge the main again and please share an update on the #34213 (comment).

dukenv0307 · 2024-02-19T07:20:17Z

Updating now.

dukenv0307 · 2024-02-19T08:00:45Z

It's fixed for me.

Screen.Recording.2024-02-19.at.14.59.40.mov

parasharrajat · 2024-02-20T12:08:30Z

Will check.

parasharrajat · 2024-02-23T08:54:05Z

Why the display names are not showing on the header tooltip but it is visible on the preview Tooltip?

Screen.Recording.2024-02-23.at.2.21.44.PM.mov

parasharrajat · 2024-02-23T09:07:00Z

Looks like the ones where email is showing in the header are all new accounts.

dukenv0307 · 2024-02-23T09:11:17Z

Looks like the ones where email is showing in the header are all new accounts.

So it's not a bug, right?

parasharrajat · 2024-02-23T09:13:25Z

There is a bug which is that Preview shows display names for these new accounts but the title does not. All of these accounts have displayName set.

dukenv0307 · 2024-02-23T09:36:10Z

Tried with the new account and seems like it's correct for me. Correct me if I missed something.

Screen.Recording.2024-02-23.at.16.34.03.mov

parasharrajat · 2024-02-24T10:04:42Z

There was some glitch I guess. Now it is fixed. Anyways, that glitch seems unrelated to this PR.

parasharrajat · 2024-02-24T10:04:51Z

Screenshots

🔲 iOS / native

Screen.Recording.2024-02-24.at.3.42.54.PM.mov

🔲 iOS / Safari

Screen.Recording.2024-02-24.at.3.37.19.PM.mov

🔲 MacOS / Desktop

Screen.Recording.2024-02-24.at.3.44.50.PM.mov

🔲 MacOS / Chrome

Screen.Recording.2024-02-24.at.3.35.09.PM.mov

🔲 Android / Chrome

Screen.Recording.2024-02-24.at.3.36.50.PM.mov

🔲 Android / native

Screen.Recording.2024-02-24.at.3.44.21.PM.mov

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

cead22

@dukenv0307 Can you please make sure to test all the instances of MultipleAvatars we use and their tooltips and make sure they look right (and add to the tests in the PR)?

cead22 · 2024-02-26T20:02:27Z

src/libs/ReportUtils.ts

+ * Return the concatenated string of the display name of a list user
+ */
+function getUserDetailsTooltipText(accountIDs: number[]): string {
+    return accountIDs.map((accountID) => getDisplayNameForParticipant(accountID)).join(', ');


NAB, but out of curiosity, would this work in TS? Maybe once it's converted to JS it does the same thing?

return accountIDs.map(getDisplayNameForParticipant).join(', ');

because the map function has two params so if we do like this, the error appears.

Which error? just curious if you know off the top of your head, otherwise I can try it myself. I thought it would work and the second param would simply be undefined

src/libs/ReportUtils.ts

cead22 · 2024-02-26T20:05:09Z

src/components/MultipleAvatars.tsx

@@ -106,7 +107,6 @@ function MultipleAvatars({
    let avatarContainerStyles = StyleUtils.getContainerStyles(size, isInReportAction);
    const {singleAvatarStyle, secondAvatarStyles} = useMemo(() => avatarSizeToStylesMap[size as AvatarSizeToStyles] ?? avatarSizeToStylesMap.default, [size, avatarSizeToStylesMap]);

-    const tooltipTexts = useMemo(() => (shouldShowTooltip ? icons.map((icon) => icon.name) : ['']), [shouldShowTooltip, icons]);


@parasharrajat was your concern here addressed/resolved?

Co-authored-by: Carlos Alvarez <cead22@gmail.com>

dukenv0307 · 2024-02-27T04:38:12Z

Just found a bug in this PR when we invite a new user to a workspace. In WorkspaceInviteMessagePage, the tooltip of the new user displays empty, the reason is this user doesn't have personal details in Onyx. So I suggest keeping icon.name and using this as the fallback in getUserDetailTooltipText like this. What do you think @parasharrajat cc @cead22.

/**
 * Returns the the display names of the given user accountIDs
 */
function getUserDetailTooltipText(accountID: number, fallbackUserDisplayName = ''): string {
    const displayNameForParticipant = getDisplayNameForParticipant(accountID);
    return displayNameForParticipant || fallbackUserDisplayName;
}

With this in MultipleAvatar we only need to change tooltipTexts as suggestion here #34213 (comment)

const tooltipTexts = useMemo(() => (shouldShowTooltip ? icons.map((icon) => ReportUtils.getUserDetailTooltipText(Number(icon.id), icon.name)) : ['']), [shouldShowTooltip, icons]);

parasharrajat · 2024-02-27T13:23:24Z

@dukenv0307 I think that will be fine given that we were using icon.name earlier.

With this in MultipleAvatar we only need to change tooltipTexts as suggestion here #34213 (comment)

Yes, please. Let's do it.

dukenv0307 · 2024-02-27T14:30:53Z

Yes, please. Let's do it.

@parasharrajat Updated this case.

dukenv0307 · 2024-02-27T14:36:50Z

Update the test step.

cead22 · 2024-02-27T17:56:04Z

@parasharrajat mind giving this another review and approve?

parasharrajat · 2024-02-27T17:56:31Z

Sure

parasharrajat · 2024-02-27T20:09:33Z

Tested and looks pretty good to me.

Screen.Recording.2024-02-28.at.1.38.10.AM.mov

OSBotify · 2024-02-28T21:03:11Z

✋ 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 · 2024-02-29T00:06:25Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.45-0 🚀

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

OSBotify · 2024-02-29T23:30:53Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.45-6 🚀

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

dukenv0307 added 2 commits January 10, 2024 12:21

create getUserDetailsTooltipText util

eb8677e

fix lint

ebabfc7

dukenv0307 marked this pull request as ready for review January 10, 2024 07:50

dukenv0307 requested a review from a team as a code owner January 10, 2024 07:50

melvin-bot bot requested review from parasharrajat and removed request for a team January 10, 2024 07:50

fix ts lint

83b98ea

merge main

2fa54df

dukenv0307 added 3 commits January 17, 2024 11:26

Merge branch 'main' of https://github.com/dukenv0307/App into fix/33740

abe71ed

Merge branch 'main' into fix/33740

029d6c2

display correct tooltip for the case subscription

032e9e3

parasharrajat reviewed Jan 22, 2024

View reviewed changes

merge main

07fdde2

Merge branch 'main' into fix/33740

5eb8eeb

merge main

b73ad6a

Merge branch 'main' into fix/33740

3ec1803

parasharrajat approved these changes Feb 24, 2024

View reviewed changes

melvin-bot bot requested a review from cead22 February 24, 2024 10:16

cead22 requested changes Feb 26, 2024

View reviewed changes

dukenv0307 and others added 2 commits February 27, 2024 09:41

Update src/libs/ReportUtils.ts

131d075

Co-authored-by: Carlos Alvarez <cead22@gmail.com>

Merge branch 'main' into fix/33740

9ab82b3

fix the case invite a new user to workspace

0b8d0d0

parasharrajat approved these changes Feb 27, 2024

View reviewed changes

melvin-bot bot requested a review from cead22 February 27, 2024 20:09

cead22 approved these changes Feb 28, 2024

View reviewed changes

cead22 merged commit 5d737d0 into Expensify:main Feb 28, 2024
19 checks passed

github-actions bot mentioned this pull request Feb 28, 2024

Deploy Checklist: New Expensify 2024-02-28 #37459

Closed

91 tasks

melvin-bot bot mentioned this pull request Feb 29, 2024

[HOLD for payment 2024-03-07] [$500] Web- Split - Tooltip shows email instead of display name when hovering over counter in split preview #33740

Closed

6 tasks

Prioritize display name to display in tooltip text #34213

Prioritize display name to display in tooltip text #34213

Conversation

dukenv0307 commented Jan 10, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 10, 2024

parasharrajat commented Jan 16, 2024

dukenv0307 commented Jan 16, 2024

parasharrajat commented Jan 16, 2024

dukenv0307 commented Jan 18, 2024

parasharrajat commented Jan 22, 2024 • edited Loading

parasharrajat commented Jan 22, 2024

Choose a reason for hiding this comment

dukenv0307 Jan 30, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Jan 29, 2024

dukenv0307 commented Jan 29, 2024

parasharrajat commented Jan 29, 2024

parasharrajat commented Feb 5, 2024

parasharrajat commented Feb 19, 2024

dukenv0307 commented Feb 19, 2024

dukenv0307 commented Feb 19, 2024

parasharrajat commented Feb 20, 2024

parasharrajat commented Feb 23, 2024 • edited Loading

parasharrajat commented Feb 23, 2024

dukenv0307 commented Feb 23, 2024

parasharrajat commented Feb 23, 2024 • edited Loading

dukenv0307 commented Feb 23, 2024

parasharrajat commented Feb 24, 2024

parasharrajat commented Feb 24, 2024 • edited Loading

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

cead22 left a 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

dukenv0307 commented Feb 27, 2024

parasharrajat commented Feb 27, 2024

dukenv0307 commented Feb 27, 2024

dukenv0307 commented Feb 27, 2024

cead22 commented Feb 27, 2024

parasharrajat commented Feb 27, 2024

parasharrajat commented Feb 27, 2024

OSBotify commented Feb 28, 2024

OSBotify commented Feb 29, 2024

OSBotify commented Feb 29, 2024

dukenv0307 commented Jan 10, 2024 •

edited

Loading

parasharrajat commented Jan 22, 2024 •

edited

Loading

dukenv0307 Jan 30, 2024 •

edited

Loading

parasharrajat commented Feb 23, 2024 •

edited

Loading

parasharrajat commented Feb 23, 2024 •

edited

Loading

parasharrajat commented Feb 24, 2024 •

edited

Loading