Initial Setup for wallet Transfer#6852
Conversation
MelvinBot
requested review from
stitesExpensify
and removed request for
a team
|
I have used the same test steps and video as the main PR. I am not sure what should I put there. There is nothing to test on this PR. |
|
Let's just tag this as |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks good I just have a few basic suggestions and will merge as soon as they are resolved.
| }, | ||
| { | ||
| Component: ChooseTransferAccountPage, | ||
| name: 'Settings_Choose_Transfer_Account', |
There was a problem hiding this comment.
I feel we could make it clearer that this is related to the Payments page and that would be a good pattern to follow.
We have SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT but the name for this is just Settings_Choose_Transfer_Account and should probably be changed to Settings_Payments_Choose_Transfer_Account
There was a problem hiding this comment.
Ok. I was thinking the same.
parasharrajat
changed the title
|
Is there any specific regression testing that we need to do? Or do we always do the whole app @marcaaron ? |
|
@parasharrajat looks like there are some conflicts :( |
| { | ||
| Permissions.canUseWallet(this.props.betas) && <CurrentWalletBalance /> | ||
| } | ||
| {Permissions.canUseWallet(this.props.betas) && ( |
There was a problem hiding this comment.
Just to double check, regular users aren't on this beta right? So this button won't show for them? Just want to make sure we're not leading people to a blank page
There was a problem hiding this comment.
I am not sure about it yet but I can remove this until the next PR.
There was a problem hiding this comment.
All of expensify.com and a handful of testers are on the beta.
There was a problem hiding this comment.
so I should remove it for now.
There was a problem hiding this comment.
FWIW this is what someone will see if they are not on the beta
So the Transfer Balance button won't appear but they do see a weird message and are able to access the "Payments" screen via "Settings"
There was a problem hiding this comment.
so I should remove it for now.
I think we could have just left it, but it's fine to add it in another PR
No specific regression testing that I know of but the whole app should be tested. At a minimum, we can just make sure that the payments list is still rendering correctly since that is the only thing |
|
Updated & removed the Transfer link until next PR. |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
LGTM. @parasharrajat please update the test steps.
parasharrajat
changed the title
|
✋ 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 @marcaaron in version: 1.1.23-2 🚀
|
|
🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀
|
4 participants
Details
Please review @marcaaron
Do a simple PR to scaffold out some of the little changes first and add all the assets, consts, pages, etc:
Add consts
Add Routes
Add blank pages for the new routes we are adding
Translations
API.js change
Move payment methods list building logic into PaymentUtils without modifying the original logic and use in PaymentMethodList.
Fixed Issues
$ #3922
Tests | QA Steps
Tested On
Screenshots
Web
output_file.mp4
Mobile Web
Desktop
iOS
Android