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 2021-11-04] Don't show drafts in LHN until after a user has left a chat with content still in the compose box. #5166

Closed
mallenexpensify opened this issue Sep 9, 2021 · 32 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Sep 9, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Coming off this issue #4615
cc @meetmangukiya , @vitHoracek

Action Performed:

Start typing with a user who is not currently at the top of you LHN

Expected Result:

The LHN doesn't change and move the chat to the top showing you're currently typing a message.

See notes for more context.

Actual Result:

LHN 'flashes' and loads the current chat at the top of LHN, which is distracting and unnecessary.

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Notes:

  1. When someone is composing a text and there are characters in the compose box, 'do nothing'.
  2. If a users leaves a chat and there are characters still in the box (aka a draft message), then move it up to the top of LHN. That way it wouldn't 'flash' and distract a user when they're in the middle of composing a message. I don't think there's much value in seeing a draft message while you're composing, only once

Version Number: Version 1.0.94-1
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1631062473055500

View all open jobs on GitHub

@mallenexpensify mallenexpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Sep 9, 2021
@MelvinBot
Copy link

Triggered auto assignment to @muttmuure (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 9, 2021
@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor Author

@vitHoracek , didn't want to assign to you but feel free to swap yourself out for @Gonals and/or the CME if you want.

@mallenexpensify
Copy link
Contributor Author

@meetmangukiya will have 'first dibs' on fixing this issue because they completed the other, related issue and have confirmed they're interested in working on this improvement to the original issue.

@mountiny
Copy link
Contributor

mountiny commented Sep 9, 2021

@mallenexpensify Thank you, I can take it on since I have the context from the issue this came from.

This can be worked on by External contributor.

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Sep 9, 2021
@mallenexpensify
Copy link
Contributor Author

@meetmangukiya can you submit a proposal here so I can hire you? And confirm in a comment here once you have?
https://www.upwork.com/jobs/~0188b3f5cc094d8f7e

@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Sep 10, 2021
@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor Author

Sorry @Beamanator , @mountiny's got this one. He must not officially be on the CME team which is why you were added.

@meetmangukiya
Copy link
Contributor

Applied on upwork @mallenexpensify

@parasharrajat
Copy link
Member

parasharrajat commented Sep 10, 2021

Proposal

If we want to Hide the Draft pencil icon until the user is active on the report and stop it from flashing from the sidebar.

  1. Pass a new argument to the CreateOption Method named as activeReportID.
  2. Update the hasDraftComment Check in the createOption
    hasDraftComment: _.size(reportDraftComment) > 0,
    .

to

        hasDraftComment: (report.reportID !== activeReportID) && _.size(reportDraftComment) > 0,
  1. Update all instances of CreateOption Call to pass the activeReportID parameter.

Note: Sorry, I think I didn't understand the task correctly. Updating the proposal...

If we only want to remove the flash on the sidebar

  1. We need to update this check so that it does not prioritize the active report.
    } else if (prioritizeReportsWithDraftComments && reportOption.hasDraftComment) {
        if(prioritizeReportsWithDraftComments && reportOption.reportID !== activeReportID && reportOption.hasDraftComment)

@mountiny
Copy link
Contributor

mountiny commented Sep 13, 2021

@meetmangukiya Could you please provide the proposal here as well? Without that, we cannot hire you. Thank you 🙇

I have seen you have wrote a comment in the issue this came from here, but that cannot be considered as a full proposal unfortunately.

@mountiny
Copy link
Contributor

Thank you for your proposal @parasharrajat! We have promised first go for @meetmangukiya as this is coming from an issue they worked on. In the comment linked above, they have proposed slightly different approach then your proposal, so I will see what their written up proposal will look like and then choose the best!

Also @parasharrajat, just to clarify, what we want to achieve here and you have probably understood it in the second updated proposal. We do not want to hide the pencil colour, we basically want to change the moment we reorder the list with a new draft to a moment we switch between reports and not while someone is typing as the flash is distracting.

@mallenexpensify
Copy link
Contributor Author

Hired @meetmangukiya in Upwork!

@mallenexpensify
Copy link
Contributor Author

n6 hold, will get to soon....

@mallenexpensify
Copy link
Contributor Author

n6 hold still in effect

@MelvinBot MelvinBot removed the Overdue label Oct 6, 2021
@parasharrajat
Copy link
Member

I am reviewing the PR and I may have found an issue. #5317 (comment)

@kadiealexander
Copy link
Contributor

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

@mountiny
Copy link
Contributor

This is not Overdue. PR is in a review.

@MelvinBot MelvinBot removed the Overdue label Oct 15, 2021
@mountiny mountiny added the Reviewing Has a PR in review label Oct 15, 2021
@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 20, 2021
@mallenexpensify
Copy link
Contributor Author

Assigned to @meetmangukiya (forgot to earlier), n6-hold is lifted, label removed!

@mountiny
Copy link
Contributor

For anyone following this issue, here is a new issue, which stems out of this #6050. It is not exactly regression as we wanted this behaviour but real use showed it is really weird and confusing, so we want to update it once more. cc @meetmangukiya

@meetmangukiya
Copy link
Contributor

Haha, so we want to keep the icons working, but reordering shouldn't be?

@mountiny
Copy link
Contributor

@meetmangukiya correct 😂 the problem is that it looks weird when you have the draft pencil icon visible -> send a message -> and the icon is still there until we switch reports.

I thought it might not be so odd, but it is and it was out of scope for this issue. So if you want to tackle it, feel free to submit a proposal to the linked issue:)

@meetmangukiya
Copy link
Contributor

Alright, thanks for updating me @mountiny. I will send a proposal when I find some time.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 28, 2021
@botify botify changed the title Don't show drafts in LHN until after a user has left a chat with content still in the compose box. [HOLD for payment 2021-11-04] Don't show drafts in LHN until after a user has left a chat with content still in the compose box. Oct 28, 2021
@botify
Copy link

botify commented Oct 28, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.10-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 2021-11-04. 🎊

@mallenexpensify
Copy link
Contributor Author

Paid in Upwork, added $250 n6-hold bonus. Thanks @meetmangukiya !

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 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants