Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Payments Page to show wallet balance, empty state, and more! #3890

Merged
merged 17 commits into from
Jul 21, 2021

Conversation

stitesExpensify
Copy link
Contributor

@stitesExpensify stitesExpensify commented Jul 6, 2021

Details

This PR improves the Payments page and brings it more in line with the original design.
Specifically:

  1. Adds the wallet balance
  2. Removes the wallet as a payment method to fund the wallet
  3. Adds the "Payment Methods" title
  4. Adds the empty state for the payment methods list
  5. Adds a loading indicator for when we don't have the wallet balance

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/169463

Tests / QA

  1. Create a new account
  2. Go to Settings -> Payments
  3. Make sure you see this empty state
    2021-07-06_15-30-41
  4. Log in with an account that has bank accounts/cards
  5. Make sure you see this state
    2021-07-06_15-51-16

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

2021-07-06_16-02-03
2021-07-06_16-01-21

@stitesExpensify stitesExpensify self-assigned this Jul 6, 2021
…s-finishPaymentsPage

52.74.223.119 ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==
@stitesExpensify stitesExpensify marked this pull request as ready for review July 6, 2021 22:17
@stitesExpensify stitesExpensify requested a review from a team as a code owner July 6, 2021 22:17
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team July 6, 2021 22:18
@stitesExpensify stitesExpensify changed the title Create new CurrentWalletBalance component Update Payments Page to show wallet balance, empty state, and more! Jul 6, 2021
Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides a super minor change and a quick question, the code looks great and tests well, good stuff :)

I'm wondering whether we should display a loader for the entire modal or disable the "Add Payment Method" button while we're waiting to grab the user's payment methods. Otherwise, it can cause the (kinda ugly/confusing) scenario depicted here where the user opens the dropdown but then the anchor for the dropdown moves:

2021-07-07_12-59-42.mp4

@jasperhuangg
Copy link
Contributor

@stitesExpensify Were you able to look into that issue I mentioned above?

@stitesExpensify
Copy link
Contributor Author

Hey there! Sorry for not updating you, but I'm adding a loading circle as we speak :)

@stitesExpensify
Copy link
Contributor Author

Updated!
2021-07-09_15-24-26

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jul 10, 2021

Hey @stitesExpensify, no worries! I just tested out your solution and it doesn't really seem like it fixes the problem I brought up earlier? I think we should just disable the "Add Payment Method" button until everything is loaded:

2021-07-10_09-30-59.mp4

@Expensify/design What are your thoughts?

@shawnborton
Copy link
Contributor

Ah, I see. That makes sense to me. I think we could just "disable" the Add Payment row and make it like 50% opacity like we do with disabled buttons. Thoughts on something like that?

@jasperhuangg
Copy link
Contributor

Ah, I see. That makes sense to me. I think we could just "disable" the Add Payment row and make it like 50% opacity like we do with disabled buttons. Thoughts on something like that?

Yup! I totally agree, that was my original recommendation. Should be a one-liner too.

cc @stitesExpensify

@stitesExpensify
Copy link
Contributor Author

Updated to disable the input when loading :)

@jasperhuangg
Copy link
Contributor

Hey @stitesExpensify sorry for the delay!

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stitesExpensify Looks good! Let's also get rid of the smaller loading spinner you added since it seems kinda redundant now?

@stitesExpensify
Copy link
Contributor Author

since it seems kinda redundant now

I disagree, I think that showing when the methods are loading is still a useful UI element. A disabled button doesn't show that things are loading in and of itself. I think the loader should stay

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jul 20, 2021

@stitesExpensify Sorry for the late response; I've been traveling back to LA. I know this PR is really simple and it's definitely been ongoing for a long time, so sorry for that as well!

I still don't really understand why we need both loaders still. Correct me if I'm wrong, but it looks like both loaders should disappear simultaneously once getPaymentMethods returns and updates all the Onyx keys that both loaders depend on in multiSet. Therefore, the original, large loader at the top of the page should suffice.

Also, from a pure UX standpoint, I think we should get someone from @Expensify/design to help us decide on how it looks. Personally, I think there should only be one loader for everything on the page.

Here's the screen recording (with both loaders) for reference:
https://user-images.githubusercontent.com/31285285/125147996-d2d23180-e161-11eb-9fda-f756f9079b99.mp4

@shawnborton
Copy link
Contributor

Candidly I'm a little lost on what is going on here, but looking at the video that Jasper just posted, I agree that the two loading spinners feel strange. Could we just have one that loads everything?

@stitesExpensify
Copy link
Contributor Author

🤦 You're right. Totally my bad, sorry. I forgot that the wallet balance also has a loader. I think we should just keep the balance loader and get rid of the smaller one. Sound good @shawnborton ?

@shawnborton
Copy link
Contributor

Sounds good!

@stitesExpensify
Copy link
Contributor Author

Sorry again about that @jasperhuangg. Updated!

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for your patience with the review :)

@jasperhuangg jasperhuangg merged commit 683bcf4 into main Jul 21, 2021
@jasperhuangg jasperhuangg deleted the stites-finishPaymentsPage branch July 21, 2021 00:50
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants