Refactored WorkspacePageWithSection#21917
Conversation
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
…308_refactor_WorkspacePageWithSections
| useEffect(() => { | ||
| fetchData(); | ||
| }, [fetchData]); | ||
|
|
||
| fetchData() { | ||
| if (this.props.shouldSkipVBBACall) { | ||
| useEffect(() => { | ||
| if (network.isOffline) { | ||
| return; | ||
| } | ||
|
|
||
| BankAccounts.openWorkspaceView(); | ||
| } | ||
|
|
||
| render() { | ||
| const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', ''); | ||
| const hasVBA = achState === BankAccount.STATE.OPEN; | ||
| const isUsingECard = lodashGet(this.props.user, 'isUsingExpensifyCard', false); | ||
| const policyID = lodashGet(this.props.route, 'params.policyID'); | ||
| const policyName = lodashGet(this.props.policy, 'name'); | ||
|
|
||
| return ( | ||
| <ScreenWrapper | ||
| includeSafeAreaPaddingBottom={false} | ||
| shouldEnablePickerAvoiding={false} | ||
| fetchData(); | ||
| }, [fetchData, network]); | ||
|
|
There was a problem hiding this comment.
I think this can be improved. This is not making sense to me currently.
There was a problem hiding this comment.
Hello @allroundexperts
Nice to meet you.
Thanks for your commenting.
Let me explain.
First useEffect function is for when component was mounted.
And second useEffect function is for when component was updated.
If we combine these 2 useEffect functions, fetchData function won't be called when component is mounted.
That means, fetchData function must be called at once without any relation with props changes.
So I defined first useEffect function.
Second useEffect function must be called whenever props is changed (but is called when network is online).
Thanks again.
allroundexperts
left a comment
allroundexperts
left a comment
There was a problem hiding this comment.
Rest of the changes look straight forward. Conditions inside componentDidUpdate need fine tuning.
|
@thesahindia I've done an initial review. If you want, you can take over from here. Else, please comment in the ticket and I'll handle the rest! |
|
@allroundexperts @thesahindia |
|
Hello, @allroundexperts |
Apologies. I was supposed to review it yesterday. Will do that today or over the weekend. |
| useEffect(() => { | ||
| fetchData(); | ||
| }, [fetchData]); | ||
|
|
||
| fetchData() { | ||
| if (this.props.shouldSkipVBBACall) { | ||
| useEffect(() => { | ||
| if (network.isOffline) { | ||
| return; | ||
| } | ||
|
|
||
| BankAccounts.openWorkspaceView(); | ||
| } | ||
|
|
||
| render() { | ||
| const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', ''); | ||
| const hasVBA = achState === BankAccount.STATE.OPEN; | ||
| const isUsingECard = lodashGet(this.props.user, 'isUsingExpensifyCard', false); | ||
| const policyID = lodashGet(this.props.route, 'params.policyID'); | ||
| const policyName = lodashGet(this.props.policy, 'name'); | ||
|
|
||
| return ( | ||
| <ScreenWrapper | ||
| includeSafeAreaPaddingBottom={false} | ||
| shouldEnablePickerAvoiding={false} | ||
| fetchData(); | ||
| }, [fetchData, network]); |
There was a problem hiding this comment.
I think this would more accurately mimic the behaviour in the previous class based component and will simplify the logic a lot as well:
useEffect(() => {
if (shouldSkipVBBACall || isOffline) {
return;
}
BankAccounts.openWorkspaceView();
}, [shouldSkipVBBACall, isOffline]);
There was a problem hiding this comment.
Hello, @allroundexperts
I used the useCallback for fetchData function for reducing reload and multiple define/execute fetchData function.
So I think we don't need shouldSkipVBBACall in useEffect.
Please let me know your thought.
Thanks.
There was a problem hiding this comment.
I would suggest using the useOnNetworkReconnect hook for this case :
useOnNetworkReconnect(() => BankAccounts.openWorkspaceView());
allroundexperts
left a comment
allroundexperts
left a comment
There was a problem hiding this comment.
Looks good mostly. One small optimisation needed.
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-07-11.at.11.15.59.PM.movMobile Web - ChromeScreen.Recording.2023-07-11.at.11.23.02.PM.movMobile Web - SafariScreen.Recording.2023-07-11.at.11.21.15.PM.movDesktopScreen.Recording.2023-07-11.at.11.18.54.PM.moviOSScreen.Recording.2023-07-11.at.11.21.55.PM.movAndroidScreen.Recording.2023-07-11.at.11.24.40.PM.mov |
| useEffect(() => { | ||
| fetchData(); | ||
| }, [fetchData]); | ||
|
|
||
| fetchData() { | ||
| if (this.props.shouldSkipVBBACall) { | ||
| useEffect(() => { | ||
| if (network.isOffline) { | ||
| return; | ||
| } | ||
|
|
||
| BankAccounts.openWorkspaceView(); | ||
| } | ||
|
|
||
| render() { | ||
| const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', ''); | ||
| const hasVBA = achState === BankAccount.STATE.OPEN; | ||
| const isUsingECard = lodashGet(this.props.user, 'isUsingExpensifyCard', false); | ||
| const policyID = lodashGet(this.props.route, 'params.policyID'); | ||
| const policyName = lodashGet(this.props.policy, 'name'); | ||
|
|
||
| return ( | ||
| <ScreenWrapper | ||
| includeSafeAreaPaddingBottom={false} | ||
| shouldEnablePickerAvoiding={false} | ||
| fetchData(); | ||
| }, [fetchData, network]); |
There was a problem hiding this comment.
I would suggest using the useOnNetworkReconnect hook for this case :
useOnNetworkReconnect(() => BankAccounts.openWorkspaceView());| WorkspacePageWithSections.displayName = 'WorkspacePageWithSections'; | ||
|
|
||
| export default compose( | ||
| withLocalize, |
There was a problem hiding this comment.
Seems like localize is not used in this component, let's remove it (remove props definition as well)
There was a problem hiding this comment.
Hello, @fedirjh
For using useOnNetworkReconnect hook, if we use this hook, then we can't make fetchData function as useCallback because of React Hooks must be called in a React function component or a custom React Hook function.
So it is okay to remove useCallback from fetchData?
But lint recommends to use the useCallback.
There was a problem hiding this comment.
Why do we use useCallback for fetchData ? it was not passed down to any child component so we should use regular function.
There was a problem hiding this comment.
Because fetchData is using props.
There was a problem hiding this comment.
@olexyt fetchData is not necessary here. As stated above, we can just use a plain hook like:
useEffect(() => {
if (shouldSkipVBBACall) {
return;
}
BankAccounts.openWorkspaceView();
}, [shouldSkipVBBACall]);
There was a problem hiding this comment.
@fedirjh
If we use the useOnNetworkReconnect, we can't handle shouldSkipVBBACall.
// Line 90
if (shouldSkipVBBACall) {
return;
}
What do you think?
There was a problem hiding this comment.
@olexyt Could be just updated to
useOnNetworkReconnect(() => !shouldSkipVBBACall && BankAccounts.openWorkspaceView());There was a problem hiding this comment.
@fedirjh
Can we remove network from propsType?
It is not used if we use the useOnNetworkReconnect.
There was a problem hiding this comment.
If it's not used, it should be removed.
There was a problem hiding this comment.
@fedirjh
Updated!
Please check again and let me know your thought.
Thanks.
…308_refactor_WorkspacePageWithSections
| function WorkspacePageWithSections(props) { | ||
| const {shouldSkipVBBACall} = props; | ||
|
|
||
| useOnNetworkReconnect(() => !shouldSkipVBBACall && BankAccounts.openWorkspaceView()); |
There was a problem hiding this comment.
@olexyt We still need the componentDidMount equivalent hook that should run when component did mount, I would suggest the following :
Define fetchData outside of the component:
fetchData(shouldSkipVBBACal) {
if (shouldSkipVBBACall) {
return;
}
BankAccounts.openWorkspaceView();
}Use useEffect hook that will run when component mount
useEffect(() => {
fetchData(shouldSkipVBBACall);
// eslint-disable-next-line react-hooks/exhaustive-deps -- It should run when component mounts
}, []);Update the useOnNetworkReconnect hook to
useOnNetworkReconnect(() => fetchData(shouldSkipVBBACall));There was a problem hiding this comment.
Updated following your suggestion.
There was a problem hiding this comment.
Thank you for the quick update , it seems useOnNetworkReconnect was not updated yet.
There was a problem hiding this comment.
Sorry @fedirjh
It was my mistake.
Now, I updated it.
Thanks.
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
There are still some unused code that should be removed, specifically withLocalize and withNetwork.
@fedirjh |
| <View style={[styles.w100, styles.flex1]}>{props.children(hasVBA, policyID, isUsingECard)}</View> | ||
| </ScrollViewWithContext> | ||
| ) : ( | ||
| props.children(hasVBA, policyID, isUsingECard) |
There was a problem hiding this comment.
This code is used twice , could be dryed to
const content = children(hasVBA, policyID, isUsingECard);footer and children could be destructed from props on top
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Looks good to me. @allroundexperts Looks good for the checklist.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #16308 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Hello @arosiclair @thesahindia |
| includeSafeAreaPaddingBottom={false} | ||
| shouldEnablePickerAvoiding={false} | ||
| function WorkspacePageWithSections(props) { | ||
| const {children, footer, shouldSkipVBBACall} = props; |
There was a problem hiding this comment.
Why deconstruct these props here and not above in the params?
There was a problem hiding this comment.
Hello, @arosiclair
Nice to meet you.
It is possible to use the destructured props params?
I thought it prohibit to use destructured props params because all of function components are using props params directly.
There was a problem hiding this comment.
So what should I do for this? @arosiclair
There was a problem hiding this comment.
You're allowed to destructure props now. We changed this not too long ago. So you can rewrite the above as
function WorkspacePageWithSections({children, footer, shouldSkipVBBACall}) {There was a problem hiding this comment.
@arosiclair
I just destructured props.
Please check again.
Thanks
…308_refactor_WorkspacePageWithSections
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 1.3.40-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.40-5 🚀
|
5 participants
Details
Migrate WorkspacePageWithSection.js to function component
Fixed Issues
$ #16308
PROPOSAL: #16308 (comment)
Tests
Should be showed all of panel
Offline tests
Follow the same steps listed in the section above
QA Steps
Follow the same steps listed in the section above
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
chrome.mp4
Mobile Web - Chrome
android-chrome.mp4
Mobile Web - Safari
iOS-safari.mov
Desktop
desktop.mov
iOS
iOS-app.mov
Android
android-app.mp4