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 2023-04-26] [$1000] Tooltip functionality for the "copy to clipboard" feature in the "details" section behaves weird if clicked multiple times. #17182

Closed
1 of 6 tasks
kavimuru opened this issue Apr 8, 2023 · 56 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Apr 8, 2023

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. Tap on name at header or display picture.
  3. Select email by clicking “copy to clipboard”.
    Notice the tooltip shakes if “copy to clipboard” is clicked for couple of times.

Expected Result

The tooltip should work smoothly and not shake, even if the "copy to clipboard" feature is clicked multiple times.

Actual Result

The tooltip shakes when the "copy to clipboard" feature is clicked multiple times.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number:
Reproducible in staging?: needs reproduction
Reproducible in production?: needs reproduction
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

20230404_173147.mp4

Expensify/Expensify Issue URL:
Issue reported by: @usmantariq96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680613201279239

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0170d1b32d4879d8e5
  • Upwork Job ID: 1645505097388904448
  • Last Price Increase: 2023-04-10
@kavimuru kavimuru added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Apr 8, 2023
@MelvinBot
Copy link

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@MelvinBot
Copy link

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Overdue label Apr 10, 2023
@CortneyOfstad
Copy link
Contributor

To be honest, I cannot recreate this and I don't really see the issue in the video shared. When I click the option to copy the email address multiple times, the tooltip doesn't move.

2023-04-10_10-04-28 (1)

@melvin-bot melvin-bot bot removed the Overdue label Apr 10, 2023
@allroundexperts
Copy link
Contributor

@CortneyOfstad In the video that you've shared, I can see the issue in the first few seconds. I've clipped the portion of your video where this is happening and have attached it.

Screen.Recording.2023-04-10.at.10.45.10.PM.online-video-cutter.com.mp4

@CortneyOfstad
Copy link
Contributor

I'm going to be honest, I'm still not seeing it. But going to get engineering eyes on this just in case for them to confirm whether or not we consider this to be a bug.

@MelvinBot
Copy link

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

@allroundexperts
Copy link
Contributor

allroundexperts commented Apr 10, 2023

Thanks for your reply @CortneyOfstad!
For someone looking at this later on, I've attached before and after screenshots of the tooltip. I hope this makes the issue more clear.

Initial State
Screenshot 2023-04-10 at 10 53 50 PM

Final State
Screenshot 2023-04-10 at 10 53 59 PM

@tgolen
Copy link
Contributor

tgolen commented Apr 10, 2023

Yep, I see what you mean. Thanks for the comparison screenshots! Let's get it fixed.

@tgolen tgolen added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Apr 10, 2023
@melvin-bot melvin-bot bot changed the title Tooltip functionality for the "copy to clipboard" feature in the "details" section behaves weird if clicked multiple times. [$1000] Tooltip functionality for the "copy to clipboard" feature in the "details" section behaves weird if clicked multiple times. Apr 10, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~0170d1b32d4879d8e5

@MelvinBot
Copy link

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 10, 2023
@MelvinBot
Copy link

Current assignee @tgolen is eligible for the External assigner, not assigning anyone new.

@hasebsiddiqui
Copy link
Contributor

hasebsiddiqui commented Apr 10, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Tooltip functionality for "copy to clipboard" is acting weird when clicking multiple times

What is the root cause of that problem?

The problem lies in two components.

  1. The first component that is responsible for rendering the tooltip is src/components/ContextMenuItem.js
    The code problematic code block is below:
triggerPressAndUpdateSuccess() {
       if (this.props.isDelayButtonStateComplete) {
           return;
       }
       this.props.onPress();

       // We only set the success state when we have icon or text to represent the success state
       // We may want to replace this check by checking the Result from OnPress Callback in future.
       if (this.props.successIcon || this.props.successText) {
           this.props.toggleDelayButtonState(this.props.autoReset);
       }
}

Here in the firstif condition, if the state of button is already complete then the onPress and toggleDelayButtonState function does not get called. While it is important to not call onPress multiple times but toggleDelayButtonState should be called multiple times to reset the delay timer of the button state.

  1. The second problem is inside toggleDelayButtonState function of src/components/withDelayToggleButtonState.js HOC component.
toggleDelayButtonState(resetAfterDelay) {
            this.setState({
                isDelayButtonStateComplete: true,
            });

            if (!resetAfterDelay) {
                return;
            }

            this.resetButtonStateCompleteTimer = setTimeout(() => {
                this.setState({
                    isDelayButtonStateComplete: false,
                });
            }, 1800);
}

Here every time this function is called a new timer is created to delay the button state with this line setTimeout but the previous timeout is not cleared. So all the previous timeouts will remain in the macro tasks queue and keep changing the state of the button. So before starting a new timeout, old one should be cleared.

What changes do you think we should make in order to solve the problem?

  1. For component src/components/ContextMenuItem.js my solution is to change the first if condition and move onPress inside this condition. So that the logic remains the same and we prevent users from triggering onPress multiple times. But the delay state of the button should be triggered every time so it remains outside if condition
triggerPressAndUpdateSuccess() {
        if (!this.props.isDelayButtonStateComplete) {
            this.props.onPress(); //NEW LINE. REPLACE return; WITH THIS LINE
        }

        // We only set the success state when we have icon or text to represent the success state
        // We may want to replace this check by checking the Result from OnPress Callback in future.
        if (this.props.successIcon || this.props.successText) {
            this.props.toggleDelayButtonState(this.props.autoReset);
        }
}
  1. Now, we need to change the src/components/withDelayToggleButtonState.js HOC component. We need to clear previous timeouts before creating new one.
toggleDelayButtonState(resetAfterDelay) {
            this.setState({
                isDelayButtonStateComplete: true,
            });

            if (!resetAfterDelay) {
                return;
            }

            // Clear any existing timers
            if (this.resetButtonStateCompleteTimer) { //NEW LINE
                clearTimeout(this.resetButtonStateCompleteTimer); //NEW LINE
            } //NEW LINE

            this.resetButtonStateCompleteTimer = setTimeout(() => {
                this.setState({
                    isDelayButtonStateComplete: false,
                });
            }, 1800);
}

Heres the video proof after implementing the suggested solution.

Screen.Recording.2023-04-11.at.1.52.41.AM.mov

What alternative solutions did you explore? (Optional)

None

@tgolen
Copy link
Contributor

tgolen commented Apr 11, 2023

I love it and I think that's a very complete proposal. The root cause analysis looks spot on and the solution looks correct.

@CortneyOfstad 🟢 to hire @hasebsiddiqui and you can go ahead and start the PR.

@hasebsiddiqui
Copy link
Contributor

Thanks @tgolen Applied to job on Upwork. Will submit PR within two hours.

@thesahindia
Copy link
Member

Late here! It looks good to me as well.

@MelvinBot
Copy link

MelvinBot commented Apr 19, 2023

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:

  • [@thesahindia] The PR that introduced the bug has been identified. Link to the PR:
  • [@thesahindia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@thesahindia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@dylanexpensify] Determine if we should create a regression test for this bug.
  • [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 26, 2023
@dylanexpensify
Copy link
Contributor

@usmantariq96 can you apply to job or share Upwork info?
@thesahindia sent invite!

@thesahindia
Copy link
Member

Accepted, thanks!

@thesahindia
Copy link
Member

We can skip the checklist for this. It was a small improvement.

@dylanexpensify
Copy link
Contributor

Hi @thesahindia I think we should still perform the checklist regardless :)

@usmantariq96
Copy link

@dylanexpensify the job seems to have been expired. Please send me an offer directly.
My Upwork profile URL: https://www.upwork.com/freelancers/~0162e9ec0b876900fb

@hasebsiddiqui
Copy link
Contributor

Hi. When can I expect my payment on upwork?

@tgolen
Copy link
Contributor

tgolen commented Apr 28, 2023

I think we usually try to get them paid within 7 days, but you might want to check the contributor guidelines to see if it says something different.

@hasebsiddiqui
Copy link
Contributor

I have checked, I should have gotten it the day before yesterday. i.e 26-04-2023. I also qualify for 50% bonus.

@tgolen
Copy link
Contributor

tgolen commented Apr 28, 2023

OK, let me reach out to Dylan and check on this one.

@dylanexpensify
Copy link
Contributor

Hi there, sorry for the miss! Working on this now!

@dylanexpensify
Copy link
Contributor

@hasebsiddiqui payment sent, contract ended!
@usmantariq96 invite sent!

@usmantariq96
Copy link

@dylanexpensify I think you sent me an offer that has an incorrect amount ($1000). It should be $250 since I just reported this bug.

@melvin-bot melvin-bot bot added the Overdue label May 1, 2023
@tgolen
Copy link
Contributor

tgolen commented May 1, 2023

@dylanexpensify bump on the above ^

@melvin-bot melvin-bot bot removed the Overdue label May 1, 2023
@thesahindia
Copy link
Member

[@thesahindia] The PR that introduced the bug has been identified. Link to the PR:

It wasn't a regression so we can just checkoff the first three items.

Not sure if we should add a specific test case for this. Here are the steps if we decide creating test case -

  1. Go to any chat
  2. Open the user details by clicking on the chat header
  3. Hover over the clipboard icon
  4. Continuously click on the clipboard icon
  5. Verify that it does not go back to its initial state during the clicking process

@dylanexpensify
Copy link
Contributor

Reviewing now! Sorry, was OOO yesterday for a bank holiday!

@dylanexpensify
Copy link
Contributor

@usmantariq96 no worries, mind just accepting and then when I pay out I'll update the amount to $250?

@dylanexpensify
Copy link
Contributor

@usmantariq96 modified the offer and resent to you!

@usmantariq96
Copy link

@dylanexpensify Offer accepted.

Thanks

@dylanexpensify
Copy link
Contributor

@usmantariq96 payment sent and contract ended! Thanks again for your honesty 😄

@dylanexpensify
Copy link
Contributor

RT here, proposed for edge case bucket testing

@thesahindia
Copy link
Member

@dylanexpensify, C+ compensation is pending. My contract is still open (link)

@dylanexpensify
Copy link
Contributor

Looking for you now @thesahindia !

@dylanexpensify
Copy link
Contributor

Ah I see what happened, all sorted!

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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants