[HOLD for payment 2022-11-18] [$500] Web -Payment - Popup with payment option is hidden title for new public accounts

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to staging.new.expensify.com
2. Log in with your band new public account
3. Go to Settings - Payments
4. Click Add Payments

Expected Result:

Popup with payment option is not hidden title for new public accounts

Actual Result:

Popup with payment option is hidden title for new public accounts

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.9.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any public accounts

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Recording.1256.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @madmax330 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot]

@madmax330 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[melvin-bot]

Triggered auto assignment to @aldo-expensify (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[stephanieelliott]

Posted to Upwork: https://www.upwork.com/jobs/~01f78d3df2e9e52ada

[rushatgabhane]

Why was the popover made to open from the top?
IIRC, the popover used to open from the bottom of the button.

[aldo-expensify]

Why was the popover made to open from the top? IIRC, the popover used to open from the bottom of the button.

I guess it could be because there are views where this button is at the bottom of the screen, like this:

Maybe we should add a prop to control if it appears on top or on bottom?

[rushatgabhane]

Maybe we should add a prop to control if it appears on top or on bottom

sounds like a fine idea

[tienifr]

Proposal

Problem

In the Payment Page, We are leaving the default menu option displayed above the button

Solution

In src/pages/settings/Payments/PaymentsPage/index.js we should add a prop (positionOfMethodMenu) into BasePaymentsPage to control that the popup with payment options appears above or below the button.
And in src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
We will check positionOfMethodMenu prop and display the payment options correctly
We shouldn't fix it directly into BasePaymentsPage.js because we can reuse it later

Please view the changed code in this sample PR: tienifr#6
Result:

[Puneet-here]

Proposal

The add payment method was meant to be at the bottom but after wrapping it with the offlineIndicator its position changed.

To fix this we have to pass styles.flex1 in style and contentContainerStyle(this prop is not available right now but a PR is up for it) prop at OfflineWithFeedback

App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js

Line 311 in 7b21103

<OfflineWithFeedback onClose={() => PaymentMethods.clearWalletError()} errors={this.props.userWallet.errors} errorRowStyles={[styles.ph6, styles.pv2]}>

contentContainerStyle will be used in the style here

App/src/components/OfflineWithFeedback.js

Line 92 in 7b21103

<View style={needsOpacity ? styles.offlineFeedback.pending : {}}>

Now we have to do to something about the margin and padding because we have one more issue, the error for payment method button doesn't look good with the current spacing (check the image below)
Without any changes:

After correcting the position:

so I propose we only use styles.mb4 when this.props.userWallet.errors contains some value because we don't want the bottom margin when showing error and the error is from userWallet.

App/src/pages/settings/Payments/PaymentMethodList.js

Line 230 in 7b21103

style={[styles.mh4, styles.mb4, styles.buttonCTA]}

Or we can just simply remove the bottom margin from the button and styles.pv2 from offlineIndicator and add pb4 at the View below

App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js

Line 311 in 7b21103

<OfflineWithFeedback onClose={() => PaymentMethods.clearWalletError()} errors={this.props.userWallet.errors} errorRowStyles={[styles.ph6, styles.pv2]}>

After my solution this will be the result -

Without error

With error

[melvin-bot]

@rushatgabhane, @stephanieelliott, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rushatgabhane]

reviewing proposal

[rushatgabhane]

The add payment method was meant to be at the bottom

@Puneet-here could you please link this discussion?
Anyway, I think the button at top looks better.

[Puneet-here]

This is the PR.

[rushatgabhane]

Thanks @Puneet-here

@Expensify/design I'm curious about your throughts on the "Add payment method" button position in #11414 (comment)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[aldo-expensify]

PR merged

[Puneet-here]

@aldo-expensify, I just wanted to know if the compensation will be increased since I also pointed out the error message positioning bug and fixed it which wasn't mentioned in this issue.

[aldo-expensify]

@Puneet-here sorry, which error positioning? the error under the "Add payment method" button?

[Puneet-here]

Yes this how the error used to show up. since the button had bottom margin and the error had vertical margin there was some extra gap.

[aldo-expensify]

@Puneet-here ok, I just noticed that your proposal mentioned this.

@mallenexpensify 👋 can I get your opinion on what @Puneet-here is requesting here: #11414 (comment)

Summary:

• The issue is about moving the button "Add payment method" to the bottom of the screen
• @Puneet-here 's included a small fix for the error's margin when it appears below the button in his proposal
• @Puneet-here pointed out that the correction on the margin is not part of the original issue, so he is requesting extra payment for that.

Thanks!

[mallenexpensify]

I'm hopping OOO and am unable to review. @slafortune can you take a look? If you're unsure, I'd address in Slack to get feedback

[slafortune]

Checking here

[slafortune]

@Puneet-here - Yes, we will also compensate with a $250 bonus for that additional fix 👍

[Puneet-here]

Thank you🎉

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.26-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Move Add payment method button to the bottom #12502

If no regressions arise, payment will be issued on 2022-11-18. 🎊

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[slafortune]

@Puneet-here @rushatgabhane you've been sent the contract on Friday, let me know once you've accepted that and I can get you paid. 👍

[Puneet-here]

Applied, thanks!

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[melvin-bot]

@shawnborton, @slafortune, @rushatgabhane, @aldo-expensify, @Puneet-here Whoops! This issue is 2 days overdue. Let's get this updated quick!

[slafortune]

Paid @rushatgabhane waiting on @Puneet-here to send payment.

[Puneet-here]

Accepted the offer, thanks!

[slafortune]

I added the $250 in that was discussed - so paid 750 total 👍