-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[HOLD for payment 2023-07-10] [$1000] Web - Tooltip is not displayed if hovered the users which are down the list #21099
Comments
Triggered auto assignment to @isabelastisser ( |
Bug0 Triage Checklist (Main S/O)
|
I can reproduce this! |
Job added to Upwork: https://www.upwork.com/jobs/~018ca8b2c30604ca26 |
Current assignee @isabelastisser is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.For some elements in the search (personalDetails) the tooltip is not displayed if hovering the image What is the root cause of that problem?The searchPage has a list of OptionRows, each element uses a component called MultipleAvatars to render de image, even though the prop of For the section of personalDetails in the searchPage the icons are calculated using the method getIcons in ReportUtils.js Since there are no reports the method only returns the source of the image. What changes do you think we should make in order to solve the problem?Update getIcons and include a new parameter called defaultName function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '') {
const result = {
source: '',
type: CONST.ICON_TYPE_AVATAR,
name: '',
};
if (_.isEmpty(report)) {
result.source = defaultIcon || Expensicons.FallbackAvatar;
result.name = defaultName || '';
return [result];
} So it can be sent in this line result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.login),false,personalDetail.login); Image of solution working in personalDetails section: Video of solution working in all list: Tooltip.working.in.whole.search.results.webmWhat alternative solutions did you explore? (Optional)At the beginning, I thought this was a backend issue but after seeing Onyx I found that it was about handling the data stored. Other solution might work is in OptionRow component preprocess the data of the row and force the inclusion of name on each icon using the information that arrives in props. Something like including |
ProposalPlease re-state the problem that we are trying to solve in this issue.Web - Tooltip is not displayed if hovered the users which are down the list What is the root cause of that problem?Easy step to reproduce this bug is - Create a workspace invite non-existent user, after that try creating the group ( the new user will appear) and it will not have tooltip. App/src/libs/OptionsListUtils.js Lines 514 to 521 in 0b7455f
We call Lines 715 to 815 in 0b7455f
We return - Lines 716 to 720 in 0b7455f
as it never reaches the - Line 814 in 0b7455f
That's why What changes do you think we should make in order to solve the problem?We need to handle the case for getting the icons when the report is empty, using
What alternative solutions did you explore? (Optional) |
@sobitneupane did you have the chance to review the proposals above? Thanks! |
I like @daordonez11's proposal. We are already using |
Proposal from @daordonez11 looks good to me. 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@MelvinBot wrong assignation 🤣 |
@daordonez11 Not a wrong assignment. The proposal will be reviewed by Carlos and if Carlos approves it and assign you the task, you can start with your PR. |
Oh my bad. 😂 thanks for the explanation @sobitneupane I thought you were already doing the assignation. |
I suggest to put this on hold for #20512 which updates avatar patterns and tooltip. |
@0xmiroslav mi proposal is about the first if when no report arrives, which hasn't been touched in the PR. |
I mean @grgia since you are already focused on this topic you could just implement the solution proposed it seems it is just a border case for personalDetailsSeciton in searchpage but might fix the issue in other points |
I will be OOO until July 5, so assigning another team member. SO for reference. |
NVM already tested high traffic everything working fine! |
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one 🚀 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.35-5 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-07-10. 🎊 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.
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:
|
Hey @sobitneupane @luacmartins is the rule of the #urgency bonus specifically 72 hours? This PR was closed on the 3rd business day after assignation so I want to know if the bonus does apply on this issue. And it was due to a variable name change 😅 |
@daordonez11 yes, it's within 72h. |
Noted thanks for the info! @luacmartins |
Probably this PR or might have caused by some PR involving accountID migration.
https://expensify.slack.com/archives/C049HHMV9SM/p1688977644296659
Yes.
|
Regression Test Proposal:
Do we agree 👍 or 👎 |
Payment Requested on Expensify. |
@sobitneupane I will review and process the payment by EOD. |
@daordonez11 - what's your profile in Upwork? I can't find you there. |
I think you might have wrongly tagged me instead of @priya-zha |
@isabelastisser I have requested payment and will be paid through Expensify App. |
@priya-zha, I sent you an offer in Upwork. @sobitneupane, I don't see a payment request from you in Upwork, please accept the offer I sent you today, and I will process the payment thorough Upwork. |
|
@isabelastisser https://www.upwork.com/freelancers/~01e96b8da58232f028 here is my link! thanks |
Paid @sobitneupane |
All set! |
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:
Action Performed:
Expected Result:
Tooltip should be displayed if hovered the users which are down the list
Actual Result:
Tooltip is not displayed if hovered the users which are down the list
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.29.6
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
error-2023-06-13_10.04.37.1.mp4
Recording.777.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686629969783219
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: