[HOLD for payment 2021-11-12] LHN: Hide the pencil icon when message is sent or draft is deleted and show the icon when user started to type

[mountiny]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Type a draft message in some chat.
   • The pencil icon for draft does not show up.
2. Switch to another chat to see the pencil icon appear and then move back to the chat with draft.
3. Sent or delete the message.

Expected Result:

The pencil icon should not be visible when there is no draft in the report.

Let's separate the pencil icon behaviour from the LHN reordering. Show the pencil icon whenever users creates the draft (immediately) and hide the pencil icon as soon as the draft is either deleted or message sent (also immediately). However, keep the LHN reordering same, we do not want the LHN to reorder while typing as that is very distracting.

Actual Result:

Observe that the pencil icon is still visible in the LHN even though we have no draft at the moment.

Workaround:

User can switch to different report or reload and the pencil icon will disappear.

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @AndrewGable
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1635195542084000

View all open jobs on GitHub

[MelvinBot]

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

[SofiedeVreese]

Posted on Upwork: https://www.upwork.com/jobs/~013ddffbd27504e496

[SofiedeVreese]

@mountiny did you want to CME this one? Let me know if not, and I can remove you from the issue and assign a CME.

[marktoman]

Proposal

SidebarLinks.js

1. Rename shouldComponentUpdate to shouldReorder
2. Add:

constructor(props) {
  super(props);
  this.state = { shouldReorder: true };
}
componentWillReceiveProps(nextProps) {
  this.state.shouldReorder = this.shouldReorder(nextProps)
}

3. Pass it to the getSidebarOptions call in render():

const {recentReports} = getSidebarOptions(
  ...
  this.state.shouldReorder
);

OptionsListUtils.js

1. Add a new param to getSidebarOptions():

function getSidebarOptions(
  ...
  prioritizeReportsWithDraftComments
)

2. Replace the constant below with the param:

let sideBarOptions = {
  ...
  prioritizeReportsWithDraftComments: prioritizeReportsWithDraftComments,
};

[mountiny]

@SofiedeVreese I would like to keep this as I was CME on issue, which kind of introduced this problem and this is an improvement from it 😅 .

@marktoman Hi Mark! Thank you for your proposal, I am not sure if this proposal really solves the problem described in the issue body. Could you please elaborate a bit more and ideally include a screen recording of the behaviour being fixed/being as expected with those proposed changes? Thank you very much!

[marktoman]

@mountiny Sure. Currently shouldComponentUpdate has logic to determine if the user is typing. If yes, then it returns false so it does not update and reorder.

I have separated the reorder behavior from the update by:

1. Removing shouldComponentUpdate. Doing so makes the pencil icon appear and disappear properly.
2. Binding the logic previously in shouldComponentUpdate to the parameter determining if the list will reorder at any given moment.

expensify-.6050.mp4

[mountiny]

@marktoman Thank you for the clarification. Sorry if this was not clear form the issue description, but could we look into making the pencil icon show up/hide immediately when some text is added/removed? There is a delay now, I am not sure if that requires some more substantial changes.

Thank you very much for looking into this one!

[marktoman]

Done. I will post a complete solution soon.

expensify-.6050-2.mp4

[mallenexpensify]

Not sure if it's helpful but... the reproduction steps that I found while testing are

1. start drafting a message to a user - no pencil icon shows (as expected)
2. Command+k to open search/chat switcher start typing a user name then click esc key to return to the chat you started typing draft in
3. Observe pencil icon in Left Hand Nav (LHN) (NOT expected behavior)
4. Send message
5. Obverse pencil icon still shows in LHN when there is no text in compose box (NOT expected behavior)

Regardless... I only think the pencil icon should be shown when there is a background chat that contains text in the compose box or an uncompleted action (which I can't think of what any of those might be since uploading an attachment is it's own modal and requesting money pops out RHN)

[mountiny]

@mallenexpensify Alright, this is about what we want the behaviour to look like in the end. I remember we were discussing it before, we can update the expected behaviour, however, it would most likely complicate the solution.

I think that for now, the pencil icon appearing right when you start to type is not as distracting since it is a moment you hit the keyboard. I think we should first change it to show the icon immediately when there is some text.

If we would then find this not ideal either, we could create new issue to improve it, but I would not add it to scope of this issue.

[marktoman]

The solution brings two changes:

• Previously, determining whether the user has a draft comment was based on the actual stored text. The problem is that the draft save is debounced in order to limit re-renders. I have added a new boolean key to indicate if a given report has a draft, which does not need to be debounced.
• Preventing re-order was done by freezing the list update, which prevented refreshing the icon. Instead, we are now conditionally ordering the items independent of the list update.

[mountiny]

@marktoman Thank you for working on that proposal. I will have a look over it properly but so far I like it. Could you please apply for the job on Upwork and post here once you have done so?

Since we would make a few changes in this, we need to make sure this gets tested really well for any regressions.

@SofiedeVreese Could you please hire @marktoman on Upwork, once they apply? Thank you very much 🙇

[mountiny]

Also I noticed now this will be your first PR in this App! Great to have you onboard and I hope we can get this change to successful end 🎉

For future reference, avoid creating a PR before you are hired or assigned to the job and just write the proposal out in here, with video/image as prove it works. Once an assigned engineer will approve, feel free to start the PR:)

[marktoman]

@mountiny @SofiedeVreese Applied. Thank you, glad you like it!

[SofiedeVreese]

Hi @marktoman I've just hired you in Upwork!

[mountiny]

@marktoman Congratulations! Just as a reminder of our contributor guidelines, as a first time contributor, you can only work on one issue at a time. Once we finish this one, you can send proposals to many issues and potentially work on them in parallel.

[mountiny]

PR is in a review!

[SofiedeVreese]

Close to being deployed.

[mountiny]

@marktoman @SofiedeVreese There has been a regression detected which has been introduced in PR solving this issue. We need to resolve that. Link is slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1636082766011900

BUG: Blank screen after logout

[SofiedeVreese]

Looks like the regression's been fixed. I'm going to add a "hold for payment", 7 days from the original deploy.

[marktoman]

@SofiedeVreese To our best knowledge, the only place where the reported error or any related issue could occur has indeed been fixed. Thank you for putting this forward.

[mountiny]

@SofiedeVreese Confirming this job shoul dbe good to pay out on the 12th.

[SofiedeVreese]

Paid and closed! Thanks for your patience @marktoman.

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.14-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• LHN: Hide the pencil icon when message is sent or draft is deleted and show the icon when user started to type #6050 #6084

If no regressions arise, payment will be issued on 2021-11-22. 🎊

[SofiedeVreese]

Hmm that's weird ^ It was already deployed 🤷

[mountiny]

This is because of the PR which fixed the regression. All good here.

[mountiny]

Oh actually looking into this more, #6084 (comment) the original PR has been deployed to production just now so the comment was correct after all, so we have paid out prematurely here. It got delayed because of the week when we have not deployed. Nothing to do about it here, just noting for future.

[SofiedeVreese]

Ah darn it, ok that's good to know, thank you @mountiny !

[marktoman]

@mountiny @SofiedeVreese Just a hopefully redundant FYI: In case of a regression, I will continue being around :)

[mountiny]

@marktoman Let's hope it wont be needed 😊 but at the same time, I hope to see you working on new issues 🎉