refactor add payment dynamic #7847

phivh · 2022-02-20T15:54:29Z

Details

Added one window listener that observed resize event and re-set position of the menu, there is a reason that needs to migrate the listener into dimension method.

Fixed Issues

$ #7385

Tests

Tested by access to the payment page from the setting page.

QA Steps

Tested On

Screenshots

Web

web.mp4

Mobile Web

NA

Desktop

desktop_app.mp4

iOS

NA

Android

NA

rushatgabhane

@phivh small issue here - payment popover jumps down on resizing.

Screencast.from.22-02-22.03-12-30.AM.+03.mp4

phivh · 2022-02-27T13:51:36Z

@rushatgabhane @deetergp updated!

rushatgabhane · 2022-02-28T05:00:10Z

@phivh could you please post a small explaination on what fixed the resizing bug? Thanks

rushatgabhane

@phivh really minor changes

rushatgabhane · 2022-02-28T15:29:19Z

src/components/KYCWall/BaseKYCWall.js

+        if (!this.state.transferBalanceButton) {
+            return;
+        }
+        const btnPosition = getClickedElementLocation(this.state.transferBalanceButton);


rename it to buttonPosition please

rushatgabhane · 2022-02-28T15:31:41Z

src/components/KYCWall/BaseKYCWall.js

@@ -20,26 +20,41 @@ class KYCWall extends React.Component {
        super(props);

        this.continue = this.continue.bind(this);
+        this.setMenuPosition = this.setMenuPosition.bind(this);
+        this.dimensionsSubscription = null;


This isn't needed, and can be removed

rushatgabhane · 2022-02-28T15:43:22Z

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

@@ -42,6 +42,7 @@ class BasePaymentsPage extends React.Component {
            formattedSelectedPaymentMethod: {},
            anchorPositionTop: 0,
            anchorPositionLeft: 0,
+            addPaymentMethodButton: null,


Same thing in this file. This isn't needed, and can be removed

I've explained the reason for the comment below, it is the same.

rushatgabhane · 2022-02-28T15:47:45Z

src/components/KYCWall/BaseKYCWall.js


        this.state = {
            shouldShowAddPaymentMenu: false,
            anchorPositionTop: 0,
            anchorPositionLeft: 0,
+            transferBalanceButton: null,


Are you sure we need to put this in state? wouldn't a class variable suffice?

@rushatgabhane The position of the menu belongs to position of the button that user clicking on. Thus, when use listener we need to re-assign this button when any click event happens, the possible way to re-assign any variables to the listener in this case needs to use state.

I know this is old, but I don't understand this explanation and it seems wrong. @rushatgabhane please try to address all comments before approving 🙇‍♂️

@marcaaron im sorry about it (and i don't understand it either)

please try to address all comments before approving

yess!

rushatgabhane · 2022-02-28T16:33:33Z

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

@@ -52,10 +53,32 @@ class BasePaymentsPage extends React.Component {
        this.deletePaymentMethod = this.deletePaymentMethod.bind(this);
        this.hidePasswordPrompt = this.hidePasswordPrompt.bind(this);
        this.navigateToTransferBalancePage = this.navigateToTransferBalancePage.bind(this);
+        this.setMenuPosition = this.setMenuPosition.bind(this);
+        this.dimensionsSubscription = null;


This isn't needed, and can be removed

rushatgabhane · 2022-02-28T16:33:51Z

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

+
+    componentWillUnmount() {
+        this.setState({
+            addPaymentMethodButton: null,


Are you sure we need to put this in state? wouldn't a class variable suffice?

rushatgabhane · 2022-02-28T16:34:14Z

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

+        if (!this.state.addPaymentMethodButton) {
+            return;
+        }
+        const btnPosition = getClickedElementLocation(this.state.addPaymentMethodButton);


rename to buttonPosition

deetergp

I don't have anything to add to what @rushatgabhane has requested already. Let's get those changes made and this will be good to go! 👍

phivh · 2022-03-01T16:06:09Z

@rushatgabhane @deetergp updated.
There are not many changes from the previous PR, the only things that only update window listener into Dimension of RN to create listener only one time when the payment page renders.

rushatgabhane

@deetergp LGTM! 🎉️

deetergp

LGTM

OSBotify · 2022-03-02T18:48:10Z

✋ 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 · 2022-03-11T01:00:56Z

🚀 Deployed to staging by @deetergp in version: 1.1.42-0 🚀

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

OSBotify · 2022-03-15T20:53:39Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

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

phivh requested a review from a team as a code owner February 20, 2022 15:54

MelvinBot requested review from deetergp and rushatgabhane and removed request for a team February 20, 2022 15:54

phivh mentioned this pull request Feb 20, 2022

Fix: Add payment dynamic #7566

Merged

11 tasks

rushatgabhane suggested changes Feb 22, 2022

View reviewed changes

phivh added 2 commits February 27, 2022 20:27

refactor add payment dynamic

440c4dd

cleanup

3f830ea

phivh force-pushed the refactor-add-payment-dynamic branch from 4f33203 to 3f830ea Compare February 27, 2022 13:49

rushatgabhane suggested changes Feb 28, 2022

View reviewed changes

deetergp requested changes Feb 28, 2022

View reviewed changes

phivh added 3 commits March 1, 2022 20:05

Merge branch 'main' into refactor-add-payment-dynamic

5073c35

cleanup

679a485

change button name

0e55025

rushatgabhane approved these changes Mar 2, 2022

View reviewed changes

deetergp approved these changes Mar 2, 2022

View reviewed changes

deetergp merged commit 8e6f9b2 into Expensify:main Mar 2, 2022

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

mountiny mentioned this pull request Mar 11, 2022

Payment - Preview is not shown when deleting paypal.me payment method #8097

Closed

phivh mentioned this pull request Mar 23, 2022

Make AddPaymentMenu dynamic #7385

Closed

phivh mentioned this pull request Jun 3, 2022

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

Closed

marcaaron mentioned this pull request Jul 15, 2022

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

Merged

89 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor add payment dynamic #7847

refactor add payment dynamic #7847

phivh commented Feb 20, 2022

rushatgabhane left a comment •

edited

Loading

phivh commented Feb 27, 2022

rushatgabhane commented Feb 28, 2022 •

edited

Loading

rushatgabhane left a comment

rushatgabhane Feb 28, 2022

rushatgabhane Feb 28, 2022

rushatgabhane Feb 28, 2022

phivh Mar 1, 2022 •

edited

Loading

rushatgabhane Feb 28, 2022

phivh Mar 1, 2022 •

edited

Loading

marcaaron Jul 15, 2022

rushatgabhane Jul 16, 2022 •

edited

Loading

rushatgabhane Feb 28, 2022

rushatgabhane Feb 28, 2022

rushatgabhane Feb 28, 2022

deetergp left a comment

phivh commented Mar 1, 2022 •

edited

Loading

rushatgabhane left a comment

deetergp left a comment

OSBotify commented Mar 2, 2022

OSBotify commented Mar 11, 2022

OSBotify commented Mar 15, 2022

refactor add payment dynamic #7847

refactor add payment dynamic #7847

Conversation

phivh commented Feb 20, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

phivh commented Feb 27, 2022

rushatgabhane commented Feb 28, 2022 • edited Loading

rushatgabhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phivh Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phivh Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane Jul 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deetergp left a comment

Choose a reason for hiding this comment

phivh commented Mar 1, 2022 • edited Loading

rushatgabhane left a comment

Choose a reason for hiding this comment

deetergp left a comment

Choose a reason for hiding this comment

OSBotify commented Mar 2, 2022

OSBotify commented Mar 11, 2022

OSBotify commented Mar 15, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Feb 28, 2022 •

edited

Loading

phivh Mar 1, 2022 •

edited

Loading

phivh Mar 1, 2022 •

edited

Loading

rushatgabhane Jul 16, 2022 •

edited

Loading

phivh commented Mar 1, 2022 •

edited

Loading