migrate workspaceReimburseView.js from class to function component#22243
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
Reviewer Checklist
Screenshots/VideosWeb22243.Web.movMobile Web - Chrome22243.mWeb.Chrome.webmMobile Web - Safari22243.mWeb.Safari.mp4Desktop22243.Desktop.moviOS22243.iOS.mp4Android22243.Android.webm |
mollfpr
left a comment
There was a problem hiding this comment.
Screen.Recording.2023-07-08.at.21.36.41.mov
The API call twice.
We can remove the fetchData inside the second useEffect and handle the fetching in the first useEffect whenever we are online.
|
@mollfpr I have made the changes, please review |
mollfpr
left a comment
There was a problem hiding this comment.
The API is still called everytime I changing the language.
Screen.Recording.2023-07-12.at.11.59.36.mov
Co-authored-by: Luthfi <luthfi.ufi14@gmail.com>
|
Hi @mollfpr , I have made the required changes, please see it |
mollfpr
left a comment
There was a problem hiding this comment.
Looks good now and tests well. Just some minor improvement.
Co-authored-by: Luthfi <luthfi.ufi14@gmail.com>
Co-authored-by: Luthfi <luthfi.ufi14@gmail.com>
|
@mollfpr the changes has been committed. Thanks for taking the pain of helping me with this as this was my first PR for Expensify. I will be more cautious from next time while making changes as well as testing. |
mollfpr
left a comment
There was a problem hiding this comment.
LGTM 👍
Sorry for taking a long time to finish this. You're doing a great job, and I like how you commit things for each purpose 👍
|
@MonilBhavsar We need a hand for the workflow and review. Thank you! |
4564cc8 to
36cf4bf
Compare
|
QA steps are missing in the PR description. They can be similar to tests. But let's QA this PR |
|
I have added the QA steps in PR description. @MonilBhavsar |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.42-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.44-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.44-2 🚀
|
Details
Fixed Issues
$ 16312
PROPOSAL: GH_LINK_ISSUE(COMMENT) 1608137455
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
WorkspaceReimburseView_web.mov
Mobile Web - Chrome
WorkspaceReimburse_android_chrome.mp4
Mobile Web - Safari
WorkspaceReimburse_ios_safari_720p.mov
Desktop
WorkspaceReimburse_desktop_720p.mov
iOS
WorkspaceReimburseView_ios_720p.mov
Android
WorkspaceReimburseView_android_480p.mov