Refactor TransferBalancePage into a functional component
#20194
Conversation
melvin-bot
bot
requested review from
0xmiros and
marcaaron
and removed request for
a team
|
@0xmiroslav @marcaaron One of you needs to 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] |
|
|
||
| /** | ||
| * Get the selected/default payment method account for wallet transfer | ||
| * @returns {Object|undefined} | ||
| */ | ||
| getSelectedPaymentMethodAccount() { | ||
| const paymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, this.props.cardList); | ||
| const getSelectedPaymentMethodAccount = useCallback(() => { |
There was a problem hiding this comment.
According to our standard, we avoid const () => pattern. Instead use function()
There was a problem hiding this comment.
👍 ah yeah, thanks for the reminder. To clarify though, you meant this for the whole component and not just this useCallback, correct?
There was a problem hiding this comment.
This is just example. Whole component, except component declaration.
I see lots of components are already refactored with const pattern, against our rule 😞
There was a problem hiding this comment.
Maybe this comment is in the wrong spot? You certainly can declare a function like this if you are calling a function that returns a function.
There was a problem hiding this comment.
yes sorry wrong spot. const can be still used in those functions like useCallback
|
|
||
| /** | ||
| * Get the selected/default payment method account for wallet transfer | ||
| * @returns {Object|undefined} | ||
| */ | ||
| getSelectedPaymentMethodAccount() { | ||
| const paymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, this.props.cardList); | ||
| const getSelectedPaymentMethodAccount = useCallback(() => { |
There was a problem hiding this comment.
Maybe this comment is in the wrong spot? You certainly can declare a function like this if you are calling a function that returns a function.
|
|
||
| /** | ||
| * Get the selected/default payment method account for wallet transfer | ||
| * @returns {Object|undefined} | ||
| */ | ||
| getSelectedPaymentMethodAccount() { | ||
| const paymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, this.props.cardList); | ||
| const getSelectedPaymentMethodAccount = useCallback(() => { |
There was a problem hiding this comment.
I think we can probably skip useCallback() here according to the latest debate on it 😄
Just inline the function and we will only worry about the optimizations if there's a good reason for it. Or if there's some good reason for it we should explain in a comment what it is. If there is, it wasn't obvious to me.
| } | ||
|
|
||
| PaymentMethods.saveWalletTransferAccountTypeAndID(selectedAccount.accountType, selectedAccount.methodID); | ||
| }, [getSelectedPaymentMethodAccount]); |
There was a problem hiding this comment.
This is... interesting... I am sort of wondering why we are running that logic in the constructor before. Because it would only ever run once for the life of the instantiated component and now it will run again whenever the reference changes (which would happen if any props change) 🤔
Not sure how to advise as I'm not sure why we used the constructor in the first place. But my first instinct would be to move this into a useEffect() with an empty array as a dependency since that is the closest thing to running logic in the constructor (but isn't exactly the same).
I haven't run into this much. It seems uncommon to run logic in the constructor unless you are initializing state.
There was a problem hiding this comment.
Yeah so for context, at first I was trying to use [] for the dependencies here to only run the effect once, but I would get the exhaustive-deps ESLint warning. Same thing if I didn't useCallback() for getSelectedPaymentMethodAccount.
I'll go ahead and try your suggestion though, thanks.
|
|
||
| /** | ||
| * @param {String} filterPaymentMethodType | ||
| */ | ||
| navigateToChooseTransferAccount(filterPaymentMethodType) { | ||
| const navigateToChooseTransferAccount = (filterPaymentMethodType) => { |
There was a problem hiding this comment.
| const navigateToChooseTransferAccount = (filterPaymentMethodType) => { | |
| function navigateToChooseTransferAccount (filterPaymentMethodType) { |
- Consolidate into a single `useEffect` with empty array dependency, to only run once on initial render - Change `useCallback` function into a regular function
francoisl
force-pushed
the
francois-transferBalanceFunctionalComponent
branch
from
a3efadd to
657ec67
Compare
|
Updated and retested! |
|
@0xmiroslav can you check this one out and give me a shout when it looks ready? 🙇 |
|
@0xmiroslav did you get a chance to review the PR again? |
|
@0xmiroslav bump please |
|
Sorry reviewing now |
| const selectedAccount = getSelectedPaymentMethodAccount(); | ||
| if (!selectedAccount) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
// Select the default payment account when page is opened,
// so that user can see that preselected on choose transfer account page
if (!selectedAccount || !selectedAccount.isDefault) {
return;
}
This was original code.
Any concerns in keeping original comment and also adding || !selectedAccount.isDefault condition?
There was a problem hiding this comment.
Yeah I think this was a bug, if we return early here and you have a non-default account selected, then we can't properly pre-select the account in the ChooseTransferAccountPage.
Here's what it looks like if we add the || !selectedAccount.isDefault (also happening on main):
Screen.Recording.2023-06-16.at.2.22.21.PM.mov
|
ok, code looks good. But I have trouble in testing this page as I have no balance. Is there any for me to test? Screen.Recording.2023-06-16.at.11.33.54.PM.mov |
|
Good question. Looking at previous PRs that modified this component, they were mostly tested internally. I asked in Slack here for now. |
|
@0xmiroslav were you able to test this PR? |
|
@francoisl I'm gonna jump in and take over the review on this one as @0xmiroslav may not be able to test. I think to get to that page you basically would need to have:
So, it seems quite hard for a C+ to test actually. I am guessing that's the explanation for the delay here. |
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - Chrome
Mobile Web - Safari
DesktopFull disclosure - ran into issues testing on Desktop and could not find time to sort them out locally. iOS
Android
|
|
Thanks @marcaaron |
|
This PR was a bit broken for me in the current state - but was fixed after merging in One thing I have noticed (also unrelated) - the button here does not go anywhere... though this feature is still behind a beta IIRC so we are good. Will continue testing on all the platforms now!
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Whoops was I not supposed to merge this? I heard something about a "merge freeze" - hopefully it is OK. |
|
Haha I forgot about that too. I think it's ok that we merged though. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.34-0 🚀
|
|
Hello @francoisl @marcaaron QA team have trouble to see the Transfer balance option under Payment. Can you please help us? Is this internal testing? |
|
@kbecciv Oh I thought you guys had Gold wallets already, my bad. I can take care of QAing this internally! |
|
Internal QA done 👍 Chrome
Desktop
Android
Android web
iOS (via Browserstack)
iOS Safari (via Browserstack)
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.34-1 🚀
|
Details
This refactors
TransferBalancePageinto a functional component.Fixed Issues
$ #16288
Tests
Prerequisites: have two accounts set up with bank accounts for money transfers.
A, send money to accountBdefaultaccount is pre-selectedOffline tests
N/A, being online is required to navigate to the Transfer Balance page
QA Steps
Prerequisites: have two accounts set up with bank accounts for money transfers.
A, send money to accountBdefaultaccount is pre-selectedPR 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
chrome2.mov
Mobile Web - Chrome
mchrome2.mov
Mobile Web - Safari
msafari2.mov
Desktop
desktop2.mov
iOS
ios2.mov
Android
android2.mov