refactored BasePaymentsPage to function based component #22412
Conversation
melvin-bot
bot
requested review from
Santhosh-Sellavel
and removed request for
a team
|
@Santhosh-Sellavel 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] |
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
| const updateShouldShowLoadingSpinner = useCallback(() => { | ||
| // In order to prevent a loop, only update state of the spinner if there is a change | ||
| const showLoadingSpinner = props.isLoadingPaymentMethods || false; | ||
| if (showLoadingSpinner !== shouldShowLoadingSpinner) { | ||
| setShouldShowLoadingSpinner(props.isLoadingPaymentMethods && !props.network.isOffline); | ||
| } | ||
| }, [props.isLoadingPaymentMethods, props.network.isOffline, shouldShowLoadingSpinner]); |
There was a problem hiding this comment.
Why this should be a callback?
There was a problem hiding this comment.
We could keep the function block alone, call it from useEffect for network/ changes and debounced Method
There was a problem hiding this comment.
Hi, @Santhosh-Sellavel
updateShouldShowLoadingSpinner and debounceSetShouldShowLoadingSpinner is called by here useEffect.
useEffect(() => {
// If the user was previously offline, skip debouncing showing the loader
if (!props.network.isOffline) {
updateShouldShowLoadingSpinner();
} else {
debounceSetShouldShowLoadingSpinner();
}
}, [props.network.isOffline, debounceSetShouldShowLoadingSpinner, updateShouldShowLoadingSpinner]);
As you can see, whenever isOffline was changed, useEffect calls different function.
And updateShouldShowLoadingSpinner has props variables like isLoadingPaymentMethods and isOffline and these variables takes part to set state in component.
So I thought it will be good to wrap in useCallback because updateShouldShowLoadingSpinner can be stacked/called to the memory multiple times whenever change isLoadingPaymentMethods and isOffline in props.
If you want to move updateShouldShowLoadingSpinner's body to move to useEffect directly, I will update it.
What do you think?
There was a problem hiding this comment.
Let it be, fine as it is!
…to 16287_refactor_BasePaymentsPage
|
Hi, @Santhosh-Sellavel |
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
…to 16287_refactor_BasePaymentsPage
| useEffect(() => { | ||
| // If the user was previously offline, skip debouncing showing the loader | ||
| if (!props.network.isOffline) { | ||
| updateShouldShowLoadingSpinner(); |
There was a problem hiding this comment.
We can use useOnNetworkReconnect hook and simplify this further
There was a problem hiding this comment.
Hi, @Santhosh-Sellavel
But we need to run a little different function per isOffline.
useEffect(() => {
// If the user was previously offline, skip debouncing showing the loader
if (!props.network.isOffline) {
updateShouldShowLoadingSpinner();
} else {
debounceSetShouldShowLoadingSpinner();
}
}, [props.network.isOffline, debounceSetShouldShowLoadingSpinner, updateShouldShowLoadingSpinner]);
As you can see, even though debounceSetShouldShowLoadingSpinner is the debounced function of updateShouldShowLoadingSpinner, they are different functions.
We need to run these different functions per network condition.
So it is some difficult to use useOnNetworkReconnect instead above useEffect.
There was a problem hiding this comment.
Let me check this
There was a problem hiding this comment.
Alright, when do you think debounceSetShouldShowLoadingSpinner method will be called?
How do you confirm whether the user was offline or not by using the current prop?
As this effect can be triggered by changing anyone of the setLoading callbacks too right? The other callbacks does not depends upon props.network alone.
There was a problem hiding this comment.
Yes, debounceSetShouldShowLoadingSpinner will be called when network is offline.
any other callbacks doesn't depends on props.network.
And this effect can be triggered by only network change.
There was a problem hiding this comment.
I think we should add offline test for this.
Theoretically, It should work, but looks like we haven't tested it, or we do't have offline test step for this case.
|
Sorry @multijump I missed submitting this yesterday! |
…to 16287_refactor_BasePaymentsPage
…sions instead of withDimensionHOC
|
Hi @Santhosh-Sellavel |
Hi, @Santhosh-Sellavel |
|
Can you please resolve conflicts |
|
Hi, @MonilBhavsar |
|
Sorry, could you please describe the test. Sorry, what is hovering off over the button triggering? |
|
@MonilBhavsar While offline, the payment button should be disabled. |
|
Actually, the wallet is hidden behind the beta. So they might have missed it. Offline Steps:
Does this look good for you @MonilBhavsar ? |
|
cc: @multijump Please update the steps thanks! |
|
Yes, that works for me. Thanks! Can we also please test it |
bump @multijump |
I've already tested this, worked well! |
Sorry for late @Santhosh-Sellavel |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Looks like we have a regression. @multijump could you please comment in this slack thread https://expensify.slack.com/archives/C049HHMV9SM/p1690547253119529?thread_ts=1690546771.892759&cid=C049HHMV9SM |
| )} | ||
| {isPayPalMeSelected && ( | ||
| <Button | ||
| onPress={navigateToAddPaypalRoute()} |
There was a problem hiding this comment.
@multijump Here is the problem, it should be
onPress={navigateToAddPaypalRoute}
There was a problem hiding this comment.
Please create a follow-up PR soon, thanks!
There was a problem hiding this comment.
Hi, @Santhosh-Sellavel
Sorry for issue.
I just created followup PR
#23816
But I couldn't record screen because paypal button didn't appear in my dev env.
Can you please let me know how can I add paypal button?
I will test and record screen.
Thanks.
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.48-0 🚀
|
| // InteractionManager fires after the currently running animation is completed. | ||
| // https://github.com/Expensify/App/issues/7768#issuecomment-1044879541 | ||
| InteractionManager.runAfterInteractions(() => { | ||
| if (Permissions.canUsePasswordlessLogins(props.betas)) { |
There was a problem hiding this comment.
Permissions.canUsePasswordlessLogins was removed in #22461
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.48-5 🚀
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.49-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.49-3 🚀
|
| } | ||
|
|
||
| BasePaymentsPage.propTypes = propTypes; | ||
| BasePaymentsPage.defaultProps = defaultProps; | ||
| BasePaymentsPage.displayName = BasePaymentsPage; |
There was a problem hiding this comment.
There was a typo here. It should have been 'BasePaymentsPage'.
And later it caused this regression.
| useEffect(() => { | ||
| PaymentMethods.openPaymentsPage(); | ||
| }, []); |
There was a problem hiding this comment.
This useEffect wasn't needed here, since another useEffect below triggers PaymentMethods.openWalletPage()
It caused #45754
Details
Migrate BasePaymentsPage to function based component
Fixed Issues
$ #16287
PROPOSAL: #16287 (comment)
Tests
Should be enabled Add Payments Method button and showed the added payment methods
Offline tests
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.mov
Mobile Web - Chrome
Android-chrome.mov
Mobile Web - Safari
iOS-safari.mov
Desktop
Desktop.mov
iOS
iOS.mov
Android
Android.mov