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

Message stays highlighted after closing the context menu #12889

Closed
kavimuru opened this issue Nov 21, 2022 · 27 comments
Closed

Message stays highlighted after closing the context menu #12889

kavimuru opened this issue Nov 21, 2022 · 27 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kavimuru
Copy link

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. Navigate to a chat
  2. Send a message
  3. Right click on the message
  4. Click anywhere at the screen to close the context menu

Expected Result:

The message shouldn't be highlighted after closing the context menu

Actual Result:

The message stays highlighted

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.29-6
Reproducible in staging?: y
Reproducible in production?: n
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.981.mp4
Screen.Recording.2022-11-19.at.3.09.05.PM.mov

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

View all open jobs on GitHub

@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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

@kavimuru kavimuru changed the title Message stays highlighted after closing the context menu reported by Message stays highlighted after closing the context menu Nov 21, 2022
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Nov 21, 2022
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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

@hungvu193
Copy link
Contributor

hungvu193 commented Nov 21, 2022

Proposal
The ReportActionItem will be highlight when it's hovered or the PopoverReportActionContextMenu is visible.
Even when the PopoverReportActionContextMenu is dismissed, the reportAction state is not getting reset so the function to check PopoverReportActionContextMenu is isActiveReportAction still return true.

Solution
Reset the reportAction state to {} when PopoverReportActionContextMenu is dismissed.
So inside PopoverReportActionContextMenu udpate the hideContextMenu:

    /**
     * Hide the ReportActionContextMenu modal popover.
     * @param {Function} onHideActionCallback Callback to be called after popover is completely hidden
     */
    hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
            selection: '',
            reportActionDraftMessage: '',
            isPopoverVisible: false,
+           reportAction: {},
        });
    }

Result:

Screen.Recording.2022-11-21.at.23.01.58.mov

@amyevans
Copy link
Contributor

This is a regression caused by #12735, cc @deetergp @Pujan92 @mananjadhav @Julesssss.

I'll open a revert of that PR.

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2022

Proposal

Root cause:
This is a regression from #12735 which fixes #11086
So selected report id is not cleared when close PopoverReportActionContextMenu modal and this makes this flag still true even after modal fully closed

Solution: clear report id only after modal fully closed
Original logic was to clear report id as soon as modal close event triggered and this caused #11086 because report id is already cleared before modal fully closed.
This fixes both this GH and #11086

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2022

Here's code proposal for this solution - clear report id only after modal fully closed

/**
* After Popover hides, call the registered onPopoverHide & onPopoverHideActionCallback callback and reset it
*/
runAndResetOnPopoverHide() {
this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
}

    /**
     * After Popover hides, call the registered onPopoverHide & onPopoverHideActionCallback callback and reset it
     */
    runAndResetOnPopoverHide() {
+       this.state.reportID = '0';
+       this.state.reportAction = {};
        this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
        this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
    }

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2022

@amyevans my solution is based on latest main which applied #12735
So I suggest to raise new PR alternatively to fix both this GH and #11086 (no need to revert regression PR)

@amyevans
Copy link
Contributor

Okay, let me review your suggestion a bit locally @0xmiroslav, thanks. The one thing I'm sensitive to is that the regression is currently a deploy blocker, so we'd need to cherry pick a fix today, and I want to avoid introducing any further regressions by not fully considering unintended side effects if moving too quickly.

@amyevans
Copy link
Contributor

Discussed with @AndrewGable and I'm going to proceed with opening a revert PR to keep the deploy unblocked. @0xmiroslav, feel free to suggest your proposal on the original issue (#11086) for evaluation by the C+ and CME, as the issue will need an updated solution after the revert. Thank you!

@amyevans amyevans added the Reviewing Has a PR in review label Nov 21, 2022
@AndrewGable AndrewGable removed the DeployBlockerCash This issue or pull request should block deployment label Nov 21, 2022
@AndrewGable
Copy link
Contributor

Removing label as the revert was CP-ed

@mananjadhav
Copy link
Collaborator

Thanks @amyevans and @AndrewGable. We'll fix it as a regression on #11086

@flaviadefaria
Copy link
Contributor

Are we then good to close this?

@amyevans
Copy link
Contributor

Yes, sorry, I forgot App issues aren't auto-closed when the related PR is deployed!

@thesahindia
Copy link
Member

@amyevans, can we re-open? This is eligible for reporting compensation.

@amyevans
Copy link
Contributor

Right, apologies @thesahindia! @flaviadefaria can you issue payment to @thesahindia please?

@amyevans amyevans reopened this Nov 28, 2022
@flaviadefaria
Copy link
Contributor

@thesahindia I'm just clarifying something internally but will aim to pay you tomorrow!

@trjExpensify trjExpensify added the Internal Requires API changes or must be handled by Expensify staff label Nov 30, 2022
@flaviadefaria
Copy link
Contributor

@thesahindia can you please apply for the job so that I can pay you?
Here's the link to the post -https://www.upwork.com/jobs/~01a04fbcc8b754d462
Please let me know once you've applied!

@thesahindia
Copy link
Member

@flaviadefaria, the job is private.
Screenshot 2022-11-30 at 5 29 06 PM

@flaviadefaria
Copy link
Contributor

flaviadefaria commented Nov 30, 2022

Weird can you try again now? It was indeed private but I've made it public now.

@thesahindia
Copy link
Member

Not sure what's wrong. Can you send me the offer or invite (Upwork profile)

Screen.Recording.2022-11-30.at.7.34.33.PM.mov

@flaviadefaria
Copy link
Contributor

Hm I must have messed up somewhere, I'll invite you to the job thanks for sending your profile.

@flaviadefaria
Copy link
Contributor

Ok @thesahindia you should have now received my offer!

@thesahindia
Copy link
Member

Accepted, thanks!

@flaviadefaria
Copy link
Contributor

Alright you should now have been paid. Thanks for your patience @thesahindia!

@thesahindia
Copy link
Member

Let's close this.

@flaviadefaria
Copy link
Contributor

Closing it - thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants