Re-render ReportFooter when transitioning to new chat by marcaaron · Pull Request #11853 · Expensify/App

marcaaron · 2022-10-14T16:32:11Z

Details

When we are transitioning from one report to another we must unmount the ReportActionsView and ReportFooter. We were only unmounting the ReportActionsView so the composer would show a stale draft.

Fixed Issues

$ #11840

Tests

Navigate to a chat
Leave a draft in the input
Navigate to another chat
Verify the draft from the previous chat does not persist in the new chat

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Navigate to a chat
Leave a draft in the input
Navigate to another chat
Verify the draft from the previous chat does not persist in the new chat

Verify that no errors appear in the JS console

Screenshots

Web

Kapture.2022-10-14.at.10.55.04.mp4

Mobile Web - Chrome

Kapture.2022-10-14.at.10.56.58.mp4

Mobile Web - Safari

Kapture.2022-10-14.at.10.58.10.mp4

Desktop

Kapture.2022-10-14.at.10.55.04.mp4

iOS

Kapture.2022-10-14.at.11.04.32.mp4

Android

Kapture.2022-10-14.at.11.07.07.mp4

Julesssss

Nice, this resolved the issue for me 👍

thesahindia · 2022-10-14T17:16:27Z

Works well for me.

C+ reviewed 🎀👀🎀

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

aldo-expensify

NAB: I tested and it is fixing it... but wanted to check if you considered updating the local comment stored in ReportActionCompose on componentDidUpdate if the report changes?

App/src/pages/home/report/ReportActionCompose.js

Lines 162 to 175 in 23686e4

    
           componentDidUpdate(prevProps) { 
        
               const sidebarOpened = !prevProps.isDrawerOpen && this.props.isDrawerOpen; 
        
               if (sidebarOpened) { 
        
                   toggleReportActionComposeView(true); 
        
               } 
        
               // We want to focus or refocus the input when a modal has been closed and the underlying screen is focused. 
        
               // We avoid doing this on native platforms since the software keyboard popping 
        
               // open creates a jarring and broken UX. 
        
               if (this.shouldFocusInputOnScreenFocus && this.props.isFocused 
        
                   && prevProps.modal.isVisible && !this.props.modal.isVisible) { 
        
                   this.focus(); 
        
               }

I'm thinking about this approach because I would think it is lest costly than unmounting and mounting again.

Update: lol, I had a look again and there is code trying to update the comment if the report changes in ReportActionCompose.componentDidUpdate, I wonder why it is not working 😕

App/src/pages/home/report/ReportActionCompose.js

Lines 181 to 187 in 0694245

    
               // we do not show incorrect data in there (ie. draft of message from other report). 
        
               if (this.props.report.reportID === prevProps.report.reportID) { 
        
                   return; 
        
               } 
        
               this.updateComment(this.props.comment); 
        
           }

Update 2: putting some console logs, by the time the reportID prop changes in ReportActionCompose.componentDidUpdate, the props.comment has not changed yet. I'm guessing that this is because how Onyx works and there is some delay on syncing the props with the data coming from Onyx.

thesahindia · 2022-10-14T17:51:55Z

This PR will also fix #11860

github-actions · 2022-10-14T18:08:23Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

melvin-bot · 2022-10-14T18:08:49Z

@yuwenmemon looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

yuwenmemon · 2022-10-14T18:09:43Z

I'm too fast for you @MelvinBot

Re-render ReportFooter when transitioning to new chat (cherry picked from commit dc28813)

…-11853 🍒 Cherry pick PR #11853 to staging 🍒

OSBotify · 2022-10-14T19:44:25Z

🚀 Cherry-picked to staging by @yuwenmemon in version: 1.2.15-3 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-10-14T20:42:33Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.15-3 🚀

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

OSBotify · 2023-06-15T01:44:04Z

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

re-render ReportFooter when transitioning to new chat

23686e4

marcaaron requested a review from a team as a code owner October 14, 2022 16:32

marcaaron self-assigned this Oct 14, 2022

melvin-bot bot requested review from aldo-expensify and thesahindia and removed request for a team October 14, 2022 16:32

Julesssss approved these changes Oct 14, 2022

View reviewed changes

thesahindia approved these changes Oct 14, 2022

View reviewed changes

aldo-expensify approved these changes Oct 14, 2022

View reviewed changes

yuwenmemon self-assigned this Oct 14, 2022

thesahindia mentioned this pull request Oct 14, 2022

Bug: Composer isn't getting auto focused on switching chatsreported by @Puneet-here #11860

Closed

yuwenmemon added the CP Staging label Oct 14, 2022

yuwenmemon merged commit dc28813 into main Oct 14, 2022

yuwenmemon deleted the marcaaron-enableNonLegacyDrawer branch October 14, 2022 18:08

melvin-bot bot added the Emergency label Oct 14, 2022

yuwenmemon removed the Emergency label Oct 14, 2022

OSBotify pushed a commit that referenced this pull request Oct 14, 2022

Merge pull request #11853 from Expensify/marcaaron-enableNonLegacyDrawer

d38c5a3

Re-render ReportFooter when transitioning to new chat (cherry picked from commit dc28813)

OSBotify mentioned this pull request Oct 14, 2022

🍒 Cherry pick PR #11853 to staging 🍒 #11862

Merged

OSBotify added a commit that referenced this pull request Oct 14, 2022

Merge pull request #11862 from Expensify/OSBotify-cherry-pick-staging…

da519c9

…-11853 🍒 Cherry pick PR #11853 to staging 🍒

OSBotify mentioned this pull request Oct 14, 2022

Deploy Checklist: New Expensify 2022-10-13 #11830

Closed

12 tasks

OSBotify mentioned this pull request Oct 14, 2022

Deploy Checklist: New Expensify 2022-10-14 #11873

Closed

53 tasks

OSBotify mentioned this pull request Oct 15, 2022

Deploy Checklist: New Expensify 2022-10-14 #11880

Closed

14 tasks

0xmiroslav mentioned this pull request Oct 31, 2022

[HOLD for Regression Test] Web - Chat - After refreshing the page, the input field is initially missing #11856

Closed

This was referenced Oct 31, 2022

[$250] BUG: when flipping between chats, the composer box is gone and only the skeleton UI shows. #11962

Closed

fix composer hidden while loading messages #12505

Merged

aimane-chnaif mentioned this pull request Nov 10, 2022

[HOLD for payment 2022-10-20] [$250] Spacing at the top in skeleton ui at iPad - Reported by @thesahindia #11088

Closed

OSBotify mentioned this pull request May 2, 2023

Deploy Checklist: New Expensify 2023-05-02 #18315

Closed

OSBotify mentioned this pull request Jun 15, 2023

Deploy Checklist: New Expensify 2023-06-14 #20782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-render ReportFooter when transitioning to new chat#11853

Re-render ReportFooter when transitioning to new chat#11853
yuwenmemon merged 1 commit intomainfrom
marcaaron-enableNonLegacyDrawer

marcaaron commented Oct 14, 2022 •

edited by yuwenmemon

Loading

Uh oh!

Julesssss left a comment

Uh oh!

thesahindia commented Oct 14, 2022

Uh oh!

aldo-expensify left a comment •

edited

Loading

Uh oh!

thesahindia commented Oct 14, 2022

Uh oh!

github-actions bot commented Oct 14, 2022

Uh oh!

melvin-bot bot commented Oct 14, 2022

Uh oh!

yuwenmemon commented Oct 14, 2022

Uh oh!

OSBotify commented Oct 14, 2022

Uh oh!

OSBotify commented Oct 14, 2022

Uh oh!

OSBotify commented Jun 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	componentDidUpdate(prevProps) {
	const sidebarOpened = !prevProps.isDrawerOpen && this.props.isDrawerOpen;
	if (sidebarOpened) {
	toggleReportActionComposeView(true);
	}

	// We want to focus or refocus the input when a modal has been closed and the underlying screen is focused.
	// We avoid doing this on native platforms since the software keyboard popping
	// open creates a jarring and broken UX.
	if (this.shouldFocusInputOnScreenFocus && this.props.isFocused
	&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
	this.focus();
	}

	// we do not show incorrect data in there (ie. draft of message from other report).
	if (this.props.report.reportID === prevProps.report.reportID) {
	return;
	}

	this.updateComment(this.props.comment);
	}

Conversation

marcaaron commented Oct 14, 2022 • edited by yuwenmemon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

thesahindia commented Oct 14, 2022

Uh oh!

aldo-expensify left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesahindia commented Oct 14, 2022

Uh oh!

github-actions bot commented Oct 14, 2022

Uh oh!

melvin-bot bot commented Oct 14, 2022

Uh oh!

yuwenmemon commented Oct 14, 2022

Uh oh!

OSBotify commented Oct 14, 2022

Uh oh!

OSBotify commented Oct 14, 2022

Uh oh!

OSBotify commented Jun 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marcaaron commented Oct 14, 2022 •

edited by yuwenmemon

Loading

aldo-expensify left a comment •

edited

Loading