fix: do not update sidebar for active report drafts #5317

meetmangukiya · 2021-09-17T07:46:32Z

Details

Add shouldComponentUpdate method to limit re-renders to cases where draft comment changes were made to reports other than the active report.

Fixed Issues

$ #5166

Tests

For wide width screens

Open a report, start writing something. The reports shouldn't prioritize.
Switch to another report, the previous report with the draft comment should prioritize.
Switch back to the report with draft comment, remove the comment, the report should not de-prioritize.
Switch to another report, the previous report should de-prioritize and the pencil icon should also be removed.
Open a new tab where you create a draft comment on another report, the reports should prioritize on the first tab.

For small screen widths

Reports with draft comments should always prioritize and de-prioritize as it should.

QA Steps

For wide width screens

Open a report, start writing something. The reports shouldn't prioritize.
Switch to another report, the previous report with the draft comment should prioritize.
Switch back to the report with draft comment, remove the comment, the report should not de-prioritize.
Switch to another report, the previous report should de-prioritize and the pencil icon should also be removed.
Open a new tab where you create a draft comment on another report, the reports should prioritize on the first tab.

For small screen widths

Reports with draft comments should always prioritize and de-prioritize as it should.

Tested On

Screenshots

Web

Screen.Recording.2021-09-17.at.12.55.52.PM.mov

Mobile Web

Simulator.Screen.Recording.-.iPhone.12.-.2021-09-17.at.13.14.44.mp4

Desktop

Screen.Recording.2021-09-17.at.12.59.20.PM.mov

iOS

Simulator.Screen.Recording.-.iPhone.12.-.2021-09-17.at.13.08.31.mp4

Android

Need help testing android, don't have a working local environment for android. Thanks!

mountiny

Just had one little change for the comment. Otherwise the ocde looks great! You have done a great job, thank you. 🎉

We will wait with merging this after the n6 release just to avoid any problems. Tahnk you for understanding.

src/pages/home/sidebar/SidebarLinks.js

mountiny

Sorry, meant to request changes.

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

mountiny · 2021-09-20T16:25:21Z

@meetmangukiya Thanks you very much 🙏 Putting this on hold for the time being.

parasharrajat · 2021-10-07T17:33:28Z

Hmm. I think there is an issue with this PR. @meetmangukiya

Issue: When you type the draft message. LHN will not show the draft Icon until you go to another chat and vice-versa(if you clear the draft, the icon will not be removed until to move to another chat) Currently(staging app) behavior is the exact opposite of what I told you and icon should update as soon as the draft is saved or removed.

cc: @mountiny.

mountiny · 2021-10-08T06:47:06Z

@parasharrajat Thank you for having a look into this. I am not sure if I understand the problem here, I think that is the behaviour we want (to be different than in staging), essentially we do not want to make any ui changes to LHN while user is typing (on web or desktop where you can see LHN). Therefore we will add the pencil icon or reshuffle the LHN once user switches reports as I do not need to know I have a draft report in the report I just type.

Have you meant some other problem? Sorry if I have misunderstood your comment 😅

parasharrajat · 2021-10-08T07:03:49Z

@mountiny
I expect the pencil icon to go away as soon as we clear the draft even if that report is active.

Why?
lets you are chatting with someone and the draft icon will stay forever until to go to another chat.

What I understood from the issue description is that we want to block the shuffle until we switch the chat. (but title says different story).

So IMO, a solution should

Block the shuffle until the user switches the chat which is the reason we created this issue.LHN - Display chats with an existing draft under pinned chats in LHN #4615 (comment)
Pencil icon should react based on the draft state.

Let me know if this makes sense.

mountiny · 2021-10-08T08:13:17Z

@parasharrajat I see. I agree it would be good to make the pencil icon disappear when the draft is deleted (or message sent) and not wait for reports switch.

But coming from the original problem which @mallenexpensify thought, I think the main idea is to not change anything in UI while person is composing the message. The problem now is that as you pause for a bit when typing, the LHN changes and icon is add and it is very eye-catching and you feel like you got a new message while this change is actually also not so important.

Therefore I would say we should not add the pencil icon when we are starting the draft as well, so it is not distracting (it is how it works in Signal for example). We could think of adding functionality to remove the icon when the draft is deleted, but I dont think this is so important now.

I believe we could go with this change and over time evaluate if we want to change the behaviour again to improve it.

What do you think @mallenexpensify?

meetmangukiya · 2021-10-08T09:01:17Z

Yes, I am with @mountiny, we did not want any visual feedback if draft comment changed is for current report to avoid it from being a distraction.

parasharrajat · 2021-10-08T09:17:36Z

So yeah, if we allow the pencil to be removed when the draft is cleared that should be enough.

I would say do the above change in this PR as this is currently working on Staging and with this PR we will overwrite that change and break it.

At last, it's your call.

Apart from this, I will still do a test to make sure things are working as they should.

mountiny · 2021-10-08T09:30:52Z

@parasharrajat I get your point here and I am open to making the change here if it would be easy update.

@meetmangukiya Would you be able to look into this (ie. when user has draft with icon shown and they delete the message, do NOT change the LHN order but only hide the pencil icon). Otherwise the behaviour should be the same. I would guess it will be minor update, but apologies if I am wrong. Thank you very much for your time.

meetmangukiya · 2021-10-08T10:51:24Z

I think it wouldn't be a simple implementation. Currently, we are achieving this by using shouldComponentUpdate to limit the renders. However, if we want to re-render(to show pencil). We'd have to manage it inside the render function

App/src/pages/home/sidebar/SidebarLinks.js

Lines 165 to 172 in 4547d90

    
           const {recentReports} = getSidebarOptions( 
        
               this.props.reports, 
        
               this.props.personalDetails, 
        
               this.props.draftComments, 
        
               activeReportID, 
        
               this.props.priorityMode, 
        
               this.props.betas, 
        
           );

The pencil is conditional on .hasDraftComment which should be accurate and already be set accordingly. However, to achieve this we'd have to keep track of previous order and then re-order the ordered recentReports back to the order that we previously had.

If we had to do this, I suppose we can do something like:

Store lastRecentReports in some internal class variable for having previous order on re-render
Store another class variable to indicate if this update was only to active report draft comment onlyActiveReportDraftUpdated
if onlyActiveReportDraftUpdated then re-order the newly returned recentReports the same as previous order by using the lastRecentReports' order. If not, we continue as normal, no re-ordering already ordered result.

Although this isn't a particularly good solution. Can think about more if we do decide this is what we want to do.

mallenexpensify · 2021-10-08T14:42:46Z

not change anything in UI while person is composing the message.
This is the main thing I am hoping to resolve with this. I still find it distracting when anything changes in the LHN while I'm typing a message, I always think there's a new incoming message when there isn't - this can be the shuffling of the order or the pencil icon being added/removed (currently it appears the pencil icon is being added at the same time as the shuffle)

parasharrajat

Ok. Now it seems to me that the change I requested is a low priority and After analysis, I can say that it requires a complex solution.

We can skip that for now. I tested other features of the app(IOU, incoming messages, etc) and LHN is reacting to all that so I would say the solution is good.

mountiny · 2021-10-10T17:34:56Z

Thank you @meetmangukiya and @parasharrajat for looking into this. I agree and I would say the best approach now is to just go ahead and we can always create a new issue in future to tweak the behaviour based on how it goes. Once the N6 hold is off, I will merge this!

@mallenexpensify thank you for your 2 cents here as well 🙇

mountiny · 2021-10-19T11:31:08Z

Hi @meetmangukiya, the n6-hold was lifted so we can go ahead with this one.

As this PR is a bit bigger, I would prefer if we could always merge main branch to any of the n6-hold PRs and re-test. I doubt there will be any regression here, but better safe than sorry in case some of the logic was changed without creating merge conflicts.

During the N6 rush, we have merged a lot of small (and some bigger) polish PRs internally and externally, so it is possible there will be some regressions and better to catch it now than in staging.

Thank you for understanding! This wont take long hopefully.

meetmangukiya · 2021-10-21T04:45:44Z

Web

Screen.Recording.2021-10-21.at.9.37.03.AM.mov

Desktop

Screen.Recording.2021-10-21.at.9.40.15.AM.mov

meetmangukiya · 2021-10-21T04:46:01Z

@mountiny any idea why the CI is failing?

mountiny · 2021-10-21T06:40:14Z

@meetmangukiya Thank you very much for merging that! It must be related to Rory's post here. There is some problem with Gemfile, so we have to wait for that to be fixed. But once the tests pass, I will merge this!

Thank you very much for your hard and great work on this one!

mountiny

@meetmangukiya Actually, reading over it, I feel like one little comment about why we do this would be helpful for anyone coming to this later. Feel free to tweak the comment, if you think it can be clearer.

src/pages/home/sidebar/SidebarLinks.js

mountiny

Perfect! Let's hope the tests will be fixed soon, thank you!

mountiny · 2021-10-21T19:19:00Z

@meetmangukiya Sorry to ask again, but can you please merge main again? It should fix the tests, hopefully!

mountiny

OSBotify · 2021-10-22T08:10:40Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-10-23T01:31:36Z

🚀 Deployed to staging by @mountiny in version: 1.1.8-10 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-10-28T20:56:28Z

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

meetmangukiya requested a review from a team as a code owner September 17, 2021 07:46

MelvinBot requested review from mountiny and removed request for a team September 17, 2021 07:46

fix: do not update sidebar for active report drafts

4547d90

meetmangukiya force-pushed the lhn-draft-flashes branch from 5e78c84 to 4547d90 Compare September 17, 2021 07:47

mountiny approved these changes Sep 20, 2021

View reviewed changes

src/pages/home/sidebar/SidebarLinks.js Outdated Show resolved Hide resolved

mountiny self-requested a review September 20, 2021 16:15

mountiny requested changes Sep 20, 2021

View reviewed changes

chore: update comment

01d490e

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

mountiny added the n6-hold label Sep 20, 2021

mountiny self-requested a review September 20, 2021 16:24

mountiny approved these changes Sep 20, 2021

View reviewed changes

mountiny changed the title ~~fix: do not update sidebar for active report drafts~~ [HOLD] fix: do not update sidebar for active report drafts Sep 20, 2021

parasharrajat mentioned this pull request Oct 7, 2021

[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

parasharrajat previously approved these changes Oct 10, 2021

View reviewed changes

mountiny self-requested a review October 19, 2021 11:29

mountiny changed the title ~~[HOLD] fix: do not update sidebar for active report drafts~~ fix: do not update sidebar for active report drafts Oct 19, 2021

mountiny removed the n6-hold label Oct 19, 2021

Merge remote-tracking branch 'origin/main' into lhn-draft-flashes

17755bf

mountiny requested changes Oct 21, 2021

View reviewed changes

src/pages/home/sidebar/SidebarLinks.js Show resolved Hide resolved

chore: add comments

1ed0e98

meetmangukiya dismissed parasharrajat’s stale review via 1ed0e98 October 21, 2021 07:02

mountiny previously approved these changes Oct 21, 2021

View reviewed changes

chore: fix lint errors

20ae55e

meetmangukiya dismissed mountiny’s stale review via 20ae55e October 21, 2021 07:08

mountiny self-requested a review October 21, 2021 07:10

mountiny approved these changes Oct 21, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into lhn-draft-flashes

7d53d84

meetmangukiya requested a review from mountiny October 22, 2021 05:10

mountiny approved these changes Oct 22, 2021

View reviewed changes

mountiny merged commit 886aa24 into Expensify:main Oct 22, 2021

OSBotify mentioned this pull request Oct 23, 2021

Deploy Checklist: New Expensify 2021-10-23 #6017

Closed

ctkochan22 mentioned this pull request Mar 1, 2022

Fix switching Priority Mode with an active draft message from crashing app #7907

Merged

33 tasks

tgolen mentioned this pull request Aug 30, 2022

Add unit tests for common sidebar functionality #10619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not update sidebar for active report drafts #5317

fix: do not update sidebar for active report drafts #5317

meetmangukiya commented Sep 17, 2021

mountiny left a comment

mountiny left a comment

mountiny commented Sep 20, 2021

parasharrajat commented Oct 7, 2021 •

edited

Loading

mountiny commented Oct 8, 2021

parasharrajat commented Oct 8, 2021 •

edited

Loading

mountiny commented Oct 8, 2021

meetmangukiya commented Oct 8, 2021

parasharrajat commented Oct 8, 2021 •

edited

Loading

mountiny commented Oct 8, 2021

meetmangukiya commented Oct 8, 2021

mallenexpensify commented Oct 8, 2021

parasharrajat left a comment

mountiny commented Oct 10, 2021

mountiny commented Oct 19, 2021

meetmangukiya commented Oct 21, 2021

meetmangukiya commented Oct 21, 2021

mountiny commented Oct 21, 2021

mountiny left a comment

mountiny left a comment

mountiny commented Oct 21, 2021

mountiny left a comment

OSBotify commented Oct 22, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

fix: do not update sidebar for active report drafts #5317

fix: do not update sidebar for active report drafts #5317

Conversation

meetmangukiya commented Sep 17, 2021

Details

Fixed Issues

Tests

For wide width screens

For small screen widths

QA Steps

For wide width screens

For small screen widths

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Sep 20, 2021

parasharrajat commented Oct 7, 2021 • edited Loading

mountiny commented Oct 8, 2021

parasharrajat commented Oct 8, 2021 • edited Loading

mountiny commented Oct 8, 2021

meetmangukiya commented Oct 8, 2021

parasharrajat commented Oct 8, 2021 • edited Loading

mountiny commented Oct 8, 2021

meetmangukiya commented Oct 8, 2021

mallenexpensify commented Oct 8, 2021

parasharrajat left a comment

Choose a reason for hiding this comment

mountiny commented Oct 10, 2021

mountiny commented Oct 19, 2021

meetmangukiya commented Oct 21, 2021

Web

Desktop

meetmangukiya commented Oct 21, 2021

mountiny commented Oct 21, 2021

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Oct 21, 2021

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 22, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

parasharrajat commented Oct 7, 2021 •

edited

Loading

parasharrajat commented Oct 8, 2021 •

edited

Loading

parasharrajat commented Oct 8, 2021 •

edited

Loading