Skip to content
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 2022-09-02] [$250] The unpin tooltip is clipped on Desktop #9331

Closed
mvtglobally opened this issue Jun 7, 2022 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

mvtglobally commented Jun 7, 2022

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:

  1. Open any chat
  2. Pin
  3. hover over the pin icon

Expected Result:

Text should be visible

Actual Result:

Text is not visible when hover over unpin

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Desktop App

Version Number: 1.1.71-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen Shot 2022-06-03 at 10 23 14 AM

Upwork URL: https://www.upwork.com/jobs/~010348882227b38f44
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1654273654678949

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jun 7, 2022

Triggered auto assignment to @arosiclair (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@sobitneupane
Copy link
Contributor

sobitneupane commented Jun 7, 2022

Reason:
This issue is occurring in Web as well. Also, the issue is not limited only to Pin tooltip. This issue can also be observed in other tooltips when we change language. This is because tooltip width and height is not updated with change in text once the tooltip is rendered.
Proposal:
The logic to update tooltipTextWidth is present only inside componentDidMount()

componentDidMount() {
this.setState({
tooltipTextWidth: this.textRef.offsetWidth,
});
}
.
If we add this logic in componentDidUpdate() as well, all the issues mentioned above will solve.

    componentDidMount() {
        this.setState({
            tooltipTextWidth: this.textRef.offsetWidth,
        });
    }

+    componentDidUpdate(prevProps) {
+        if (prevProps.text === this.props.text) {
+            return;
+        }
+        this.setState({
+            tooltipTextWidth: this.textRef.offsetWidth,
+        });
+    }

@Tushu17
Copy link
Contributor

Tushu17 commented Jun 7, 2022

Hey, I've reported this bug before on slack.

@luacmartins
Copy link
Contributor

@mvtglobally can you update the issue reporter to @Tushu17, since he mentioned it first? Thank you!

@arosiclair
Copy link
Contributor

Clear instructions and seems like low-hanging fruit for a contributor. Marking as external.

@arosiclair arosiclair added the External Added to denote the issue can be worked on by a contributor label Jun 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2022

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@arosiclair arosiclair added Weekly KSv2 and removed Daily KSv2 labels Jun 8, 2022
@arosiclair arosiclair removed their assignment Jun 8, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 9, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 9, 2022

Triggered auto assignment to @stitesExpensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title The unpin tooltip is clipped on Desktop [$250] The unpin tooltip is clipped on Desktop Jun 9, 2022
@trjExpensify
Copy link
Contributor

Job posted to upwork here: https://www.upwork.com/jobs/~010348882227b38f44

@mananjadhav
Copy link
Collaborator

@stitesExpensify, Yeah, the issue is if the tooltip is open and we change the text, its width doesn't change. @sobitneupane's proposal looks good.

@sobitneupane While I appreciate the spontaneous solutions, please wait for the issue to get triaged before posting the proposals.

🎀 👀 🎀 
C+ reviewed

@stitesExpensify
Copy link
Contributor

I agree that @sobitneupane proposal looks good!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 10, 2022

📣 @sobitneupane You have been assigned to this job by @stitesExpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mananjadhav
Copy link
Collaborator

@sobitneupane Were you able to check the above messages?

@sobitneupane
Copy link
Contributor

I am away from my laptop. I will try to debug within this week.

@mananjadhav
Copy link
Collaborator

@sobitneupane Did you get a chance to look at this?

@mananjadhav
Copy link
Collaborator

@sobitneupane Quick reminder. @stitesExpensify Will wait for 1-2 days else I'll raise a fix for this issue.

@trjExpensify
Copy link
Contributor

@mananjadhav @sobitneupane @stitesExpensify what's going on with this issue, it's a tad unclear on the next steps?

@mananjadhav
Copy link
Collaborator

@trjExpensify Thanks for the bump. We haven't heard from @sobitneupane and I guess its a small fix, let me raise a PR for the fix by today/tomorrow.

@trjExpensify
Copy link
Contributor

Thank you, @mananjadhav.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 23, 2022

@stitesExpensify @trjExpensify PR Raised for Safari Fix

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 26, 2022
@melvin-bot melvin-bot bot changed the title [$250] The unpin tooltip is clipped on Desktop [HOLD for payment 2022-09-02] [$250] The unpin tooltip is clipped on Desktop Aug 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 26, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.91-1 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 2022-09-02. 🎊

@sobitneupane
Copy link
Contributor

Thanks @mananjadhav for handling the issue. I was away last two months. And Sorry for the inconvenience.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 2, 2022
@adeel0202
Copy link
Contributor

Hi, I reported the bug here when it was supposed to be fixed and was in production. Am I eligible for the reporting bonus?
cc: @stitesExpensify @trjExpensify

@trjExpensify
Copy link
Contributor

Okay, can I get some clarity over what's due here and to whom? Here's my take..

$250 - @sobitneupane for the fix
$250 - @Tushu17 for the original reporting
$250 - @mananjadhav for C+
$250 - @adeel0202 for reporting the safari issue

Let me know if that's correct and I'll raise a new job on Upwork because the previous one has lapsed, and no contracts were started here.

@mananjadhav
Copy link
Collaborator

@trjExpensify There was a regression on the issue, which should deduct the amount for C+, but I would appreciate if can add an exception for this one as I put additional efforts in fixing the regression myself.

@trjExpensify
Copy link
Contributor

Yeah, you fixed it, so I kept that in. 👍

Okay, job is here to apply everyone: https://www.upwork.com/jobs/~01e6c7617b3537f422

@trjExpensify
Copy link
Contributor

@mananjadhav & @adeel0202 - paid!
@sobitneupane - offer sent
@Tushu17 - waiting for you to apply.

@trjExpensify
Copy link
Contributor

@sobitneupane - paid.

@trjExpensify
Copy link
Contributor

@Tushu17 - offer sent.

@Tushu17
Copy link
Contributor

Tushu17 commented Sep 6, 2022

@trjExpensify offer accepted. Thank you.

@trjExpensify
Copy link
Contributor

@Tushu17 - paid. Closing this out! 🎉

@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants