[HOLD for payment 2022-09-08] [$250] Settings - Payment options are overlapping with Add Payment Method under Settings

[kavimuru]

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. Log in to NewDot with any account
2. Go Setting - Payments
3. Tap on the middle "Add Payment Method"

Expected Result:

Payment options are not overlapping with Add Payment Method under Settings

Actual Result:

Payment options are overlapping with Add Payment Method under Settings

Workaround:

Visual

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.70.2

Reproducible in staging?: Y

Reproducible in production?: Y

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos:

Bug5593582_Recording__524.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 @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot]

Triggered auto assignment to @NicMendonca (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 - @mananjadhav (Exported)

[melvin-bot]

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

[NicMendonca]

Job post: https://www.upwork.com/jobs/~014470d4b94fc99a28

[allroundexperts]

Proposal

Problem

We're using event.target.getBoundingClientRect to get the placement position of the dropdown. The target can be any child of the element to which the listener is attached. As such, we're getting incorrect positions.

Solution

We should use event.currentTarget.getBoundingClientRect instead to get the correct element position.

The following changes:

https://github.com/allroundexperts/Expensify/blob/1b74843140f609fa8900d754148fa9e9e7009176/src/libs/getClickedElementLocation/index.js#L8

To

    return (nativeEvent.currentTarget || nativeEvent.target).getBoundingClientRect();

Result

Screen.Recording.2022-06-02.at.8.57.02.PM.mov

[phivh]

Proposal

To keep the position of the menu when resizing the window.

1. Add styles.pointerEventsNone to container of childrens inside the . Line 198
   

   App/src/components/Button.js

   Line 198 in d2f9d61

   <View style={[styles.justifyContentBetween, styles.flexRow]}>

<View style={[styles.justifyContentBetween, styles.flexRow, styles.pointerEventsNone]}>
....
</View>

2. Preview

• Without window resize & with window resize

2022-06-02_23-51-32.mp4

[mananjadhav]

@allroundexperts's proposal looks good to me. I can see this isn't used at other places except for KYCWall within the Payments Screen. @marcaaron What are your thoughts?

@phivh Thank you for the comment, but I adding styles.pointerEventsNone will disable all cursor options, and that too we're adding to the Button component, which gets used across the application.

🎀 👀 🎀
C+ reviewed

[phivh]

Hey @mananjadhav , maybe you are wrong, by adding styles.pointerEventsNone in the renderContent which renders inside the Button it won't affect any cursor options for the Button itself.

App/src/components/Button.js

Line 272 in 72aac1c

{this.renderContent()}

However, the Button is wrapped by Pressable and OpacityView, see:

App/src/components/Button.js

Line 230 in 72aac1c

<Pressable

App/src/components/Button.js

Line 252 in 72aac1c

<OpacityView

Sorry but the proposal of @allroundexperts only gets the position of Button once. It will cause an issue that I fixed before when the user resizes the window width of the browser, see:
Issue: #7385
Fixed by: #7847 & #7566

[allroundexperts]

@phivh This isn't the root of the problem. The root is a wrong usage of target. Also, I'm not sure how this will effect the fix you made.

[liyamahendra]

Proposal

Problem

As shared by @allroundexperts the root cause of this is the use of target instead of currentTarget in the function getClickedElementLocation method call.

event.currentTarget tells us on which element the event was attached or the element whose eventListener triggered the event whereas event.target tells where the event started.

However, there are two problems with @allroundexperts' solution:

1. setMenuPosition registered at Dimensions.addEventListener('change', this.setMenuPosition) throws an error when the browser is resized:

2. It doesn't handle the resize event of the browser - meaning, if the payment method pop-up is visible and the browser is resized, the pop-up stays in the same place.

Solution

The fix for this issue is a two part solution:

1. To store the current target from the native event inside the state for later use
2. A new folder for getClickedTargetLocation which takes care of the web and native platforms and returns the getBoundingClientRect from the passed target stored in step 1.

paymentMethodPressed(nativeEvent, accountType, account, isDefault) {
   // ++ Changes starts
    const position = getClickedTargetLocation(nativeEvent.currentTarget);

    this.setState({
        addPaymentMethodNativeEventCurrentTarget: nativeEvent.currentTarget,
        addPaymentMethodButton: nativeEvent,
    });
   // ++ Changes Ends

    if (accountType) {
        let formattedSelectedPaymentMethod;
        if (accountType === CONST.PAYMENT_METHODS.PAYPAL) {
            formattedSelectedPaymentMethod = {
                title: 'PayPal.me',
                icon: account.icon,
                description: account.username,
                type: CONST.PAYMENT_METHODS.PAYPAL,
            };
        } else if (accountType === CONST.PAYMENT_METHODS.BANK_ACCOUNT) {
            formattedSelectedPaymentMethod = {
                title: account.addressName,
                icon: account.icon,
                description: `${this.props.translate('paymentMethodList.accountLastFour')} ${account.accountNumber.slice(-4)}`,
                type: CONST.PAYMENT_METHODS.BANK_ACCOUNT,
            };
        } else if (accountType === CONST.PAYMENT_METHODS.DEBIT_CARD) {
            formattedSelectedPaymentMethod = {
                title: account.addressName,
                icon: account.icon,
                description: `${this.props.translate('paymentMethodList.cardLastFour')} ${account.cardNumber.slice(-4)}`,
                type: CONST.PAYMENT_METHODS.DEBIT_CARD,
            };
        }
        this.setState({
            isSelectedPaymentMethodDefault: isDefault,
            shouldShowDefaultDeleteMenu: true,
            selectedPaymentMethod: account,
            selectedPaymentMethodType: accountType,
            formattedSelectedPaymentMethod,
        });
        this.setPositionAddPaymentMenu(position);
        return;
    }
    this.setState({
        shouldShowAddPaymentMenu: true,
    });

    this.setPositionAddPaymentMenu(position);
}

And change the setMenuPosition function as below:

setMenuPosition() {
        if (!this.state.addPaymentMethodNativeEventCurrentTarget) {
            return;
        }
        const buttonPosition = getClickedTargetLocation(this.state.addPaymentMethodNativeEventCurrentTarget);
        this.setPositionAddPaymentMenu(buttonPosition);
    }

I also set the addPaymentMethodNativeEventCurrentTarget to null when the pop-up is dismissed:

hideAddPaymentMenu() {
        this.setState({
            addPaymentMethodNativeEventCurrentTarget: null,
            shouldShowAddPaymentMenu: false
        });
    }

Attached is the screencast (with voice) showing the issue with browser resize and the applied fix:

fix-small.mp4

@mananjadhav kindly review my proposal.

Thank you!

[liyamahendra]

Apparently the screencast uploaded in the previous message doesn't seem to play. You may view it here: https://www.veed.io/view/3dda4edd-5079-4314-8683-1386e373ac05

[allroundexperts]

The browser resize issue exists with the original implementation as well. No where in the ticket, was it mentioned that responsiveness needs to be fixed. The ticket and the video posted, mention the issue which was fixed in my PR.

[liyamahendra]

Sure, no offence please @allroundexperts.

Your did still introduce the javascript error as shared in my proposal and so I opted to propose a new solution. Cheers :)

[melvin-bot]

This issue has not been updated in over 15 days. @liyamahendra, @mananjadhav, @trjExpensify, @marcaaron eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

👋 @liyamahendra can we get a status update on the linked PR please? What's left to do and when do you expect to be able to get it over the line? Thanks!

[liyamahendra]

I think @mananjadhav gave his clearance here. There were a couple of comments after that from @marcaaron which were resolved as well.

Maybe @mananjadhav or @marcaaron can provide a status update if there is anything pending.

[trjExpensify]

Bump @mananjadhav & @marcaaron shouldn't this PR be merged by now? 🤔

[mananjadhav]

@marcaaron Wanted to check something before we merge this. If there's any changes based on his feedback, @liyamahendra and I would take this up.

[marcaaron]

Sorry, I'm not sure what is needed from me. What is it exactly that you wanted to check?

[marcaaron]

Seems like we are waiting for @liyamahendra to request another review.

[marcaaron]

I think we probably must consider this abandoned? It's been a month without an update.

[liyamahendra]

Seems like we are waiting for @liyamahendra to request another review.

@marcaaron sorry, I'm not aware what are we waiting for here from me. Happy to do the needful if you please let me know. Thanks

[mananjadhav]

@marcaaron I had reviewed the PR and approved. Waiting for your final approval.

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[marcaaron]

@mananjadhav will you review the changes again?

[marcaaron]

My expectations here are that @liyamahendra will let us know when the PR is ready for re-review and that @mananjadhav responds to the review request. So, that's what I've been waiting for :)

[liyamahendra]

@marcaaron the PR is ready for review. @mananjadhav please do the needful!

[mananjadhav]

I'll do that today.

[melvin-bot]

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

• Solved the overlapping of payment options when clicking on 'Add Payment Method' #9552

If no regressions arise, payment will be issued on 2022-09-08. 🎊

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mananjadhav]

@trjExpensify Quick bump on Upwork payment.

[trjExpensify]

Yep, I've been OOO. Sorry! So looking at the Upwork job, we've already settled up with @liyamahendra a while back. @NicMendonca might have some insight into that, but seems like we're settled:

As for C+ payment @mananjadhav, I've settled up with you.

Going to close this out, but @liyamahendra of course reopen if I didn't get something right here.

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.