[HOLD for payment 2023-08-03] [$1000] Web - The tooltip shows multiple/all the selected users when hovering over the overlayed avatar #22951
Comments
kbecciv
added
Daily
|
Triggered auto assignment to @jliexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.The tooltip shows number of users max than the reminding when Hovering over the Overlayed avatar. What is the root cause of that problem?Wrong calculate for tooltip text in App/src/components/MultipleAvatars.js Lines 195 to 198 in b9508b6 we now use rows so What changes do you think we should make in order to solve the problem?The tooltip text should be // text={tooltipTexts.slice(3, 10).join(', ')}
text={tooltipTexts.slice((avatarRows.length * props.maxAvatarsInRow) - 1, 10).join(', ')}
What alternative solutions did you explore? (Optional)we can calculate it out of tooltipTexts text={props.shouldShowTooltip ? _.pluck(avatars.slice(props.maxAvatarsInRow - 1), 'name').slice(0, 10).join(', ') : ''} |
|
Hmm, very odd - I invited 9 users and here's what I see:
So I think that hovering over the +1 should only accurately reflect who isn't currently on those 2 lines (i.e. Victoria). |
jliexpensify
changed the title
|
Oh, this is a dupe of #22685 |
|
Hi @jliexpensify I think they are different issues. The one you mentioned is about the Avatars moving to the left when adding more than 8 users(this issue can also be seen in the IOU report of more than 7 user splitting a bill and also in "workspace" in the settings). The issue I posted is about the tooltip not showing the correct users. Can you please comment on this so I can have a better understanding on how they are the same |
|
@daveSeife I'm going off the Slack link, but it looks like it might be the incorrect one associated with this GH? |
jliexpensify
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~0117cec49274e7cba8 |
melvin-bot
bot
added
the
Help Wanted
|
Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
|
Hi @aimane-chnaif - based off Ahmed's comment here, this isn't considered a regression right? It's just something that we didn't account for in the other PR? |
ProposalPlease re-state the problem that we are trying to solve in this issue.The tooltip shows number of users max than the reminding when Hovering over the Overlayed avatar. What is the root cause of that problem?The cause of this issue is wrong calculation of the number of remaing avatars to be shown in the tooltip. App/src/components/MultipleAvatars.js Lines 195 to 198 in b9508b6 This is the part of the code, which is causing the current issue. What changes do you think we should make in order to solve the problem?To fix this issue, We will define two states as follows along with a const value to show the number of emails in the tooltip const maxAvatarsInTooltip = 9; //10 - 1 in order to point to the indexes of the array;
const maxAvatarsRows = 2;
// This value will have a value of max 10 depending on the number of user's avatar in the list.
// we are limiting it to 10 or so as some reports have hundreds of users, causing performance to degrade.
const [remainingAvatars, setRemainingAvatars] = useState(0);
// We are counting the remaining number of the users which are not included in the avatars and the tooltip
// to show the count in the tooltip
const [notListedAvatarsCount, setNotListedAvatarsCount] = useState(0);then we will update the values of the states defined above in calculateAvatarRows function with this code. // We are going to have only two rows as per the code here above. So we will exclude the Avatars in the rows from the list and
// Show only 10 more users in the tooltip. If there are more users, We will add a string in the tooltip
// With remaining numbers which are not included in the tooltip.
if (props.icons.length > props.maxAvatarsInRow * maxAvatarsRows) {
const avatarsCountInTooltip = props.maxAvatarsInRow * maxAvatarsRows + maxAvatarsInTooltip;
setRemainingAvatars(avatarsCountInTooltip);
if (props.icons.length > avatarsCountInTooltip) {
setNotListedAvatarsCount(props.icons.length - avatarsCountInTooltip);
}
}Then we will use the states to show the max 10 emails in the tooltip and add the remaining numbers in the tooltip with "...+ N More" <Tooltip
// We only want to cap tooltips to only the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
text={`${tooltipTexts.slice((avatarRows.length * props.maxAvatarsInRow) - 1, remainingAvatars).join(', ')}
${notListedAvatarsCount > 0 ? ` ...+ ${notListedAvatarsCount} More`: "" }`}
> |
|
📣 @niteshletxsoft! 📣
|
|
Contributor details |
|
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
Right, we can fix this as separate here. |
|
@ahmedGaber93's proposal looks good to me.
🎀 👀 🎀 C+ reviewed |
|
Triggered auto assignment to @madmax330, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
|
Hi @aimane-chnaif I have proposed a better solution to the tooltip issue and an enhancement to handle the large number of members in a Workspace. We can also include the below two points with this task as well.
Looking forward for your reply. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
|
📣 @ahmedGaber93 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Upwork job |
|
I agree with @aimane-chnaif's recommendation |
|
📣 @daveSeife 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app! |
melvin-bot
bot
added
Reviewing
|
@aimane-chnaif PR is now ready for review. |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.46-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-08-03. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
Payment Summary:
Is this correct? Upworks Job here. Friendly bump @aimane-chnaif to complete the checklist |
|
There was weekend |
|
@aimane-chnaif can you share the timeline please, for documentation purposes? |
|
Thank you, updated above! |
|
Bump @aimane-chnaif to complete the checklist before payment. Thanks! |
Regression Test Proposal:
|
|
Everyone paid, job closed, Thanks! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The tooltip should only show users missing from the initial 2 lines
Actual Result:
The tooltip shows multiple/all the selected users.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.41-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Test75_Overlayedtooltip-1.mp4
Recording.3661.mp4
Expensify/Expensify Issue URL:
Issue reported by: @daveSeife
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689364011034509
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: