Fix: Add payment dynamic #7566

phivh · 2022-02-04T16:04:03Z

Details

The menu add payment method doesn't align when resizing the screen width. Thus, added one window listener that observed resize event and re-set position of the menu.

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 Yo, let's make this code more DRY.
Only one function is different between index.js and index.native.js file.

You can refer TextInput on how you could achieve this.

phivh · 2022-02-06T08:22:46Z

@rushatgabhane @deetergp I've updated, created the base file to make it more DRY.

rushatgabhane · 2022-02-06T13:42:22Z

src/components/Form.js

@@ -122,6 +122,7 @@ class Form extends React.Component {
     * @returns {React.Component}
     */
    childrenWrapperWithProps(children) {
+        // eslint-disable-next-line rulesdir/prefer-underscore-method
        return React.Children.map(children, (child) => {


This is not related to the issue.

@luacmartins can _.map(children, (child)=>{}) be used here instead?

@rushatgabhane It doesn't pass lint check. I guess it's not a big deal, thus I added it.
Reference: https://reactjs.org/docs/react-api.html#reactchildrenmap

@rushatgabhane great question! React.Children.map is not equivalent to _.map, as it's a special method provided to React's children.

Thanks so much!

@phivh Something is off since we added that rule to the linter here - Expensify/eslint-config-expensify#44. Make sure to pull the latest changes on main and run npm install

yeah, it couldn't have been merged to main if it didn't pass the lint check

rushatgabhane

Overall looks good, just requesting some minor changes @phivh

rushatgabhane · 2022-02-07T10:34:42Z

src/components/KYCWall/BaseKYCWall.js

@@ -52,6 +29,9 @@ class KYCWall extends React.Component {
    }

    componentDidMount() {
+        if (this.props.isResizeListen) {
+            window.removeEventListener('resize', null);


This should be in componentWillUnmount()

rushatgabhane · 2022-02-07T10:46:08Z

src/components/KYCWall/kycWallPropTypes.js

+    /** Where to place the popover */
+    popoverPlacement: PropTypes.string,
+
+    /** Is resize event happens on the web/desktop? */


Not a blocker: Change the comment to "Listen for window resize event on web and desktop."

rushatgabhane · 2022-02-07T10:46:53Z

src/components/KYCWall/kycWallPropTypes.js

+    popoverPlacement: PropTypes.string,
+
+    /** Is resize event happens on the web/desktop? */
+    isResizeListen: PropTypes.bool,


Not a blocker: change the prop name to listenResize?

rushatgabhane · 2022-02-07T10:47:53Z

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

+     *
+     * @param {Object} position
+     */
+


remove extra new line

rushatgabhane · 2022-02-07T10:49:23Z

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

                formattedSelectedPaymentMethod,
            });
+            this.setPositionAddPaymentMenu(position);
        } else {


Remove else and return early in the above if instead.

rushatgabhane · 2022-02-07T10:50:12Z

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

+    /** Are we loading payment methods? */
+    isLoadingPaymentMethods: PropTypes.bool,
+
+    /** Is resize event happens on the web/desktop? */


Not a blocker: Change the comment to "Listen for window resize event on web and desktop."

rushatgabhane · 2022-02-07T10:50:27Z

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

+    isLoadingPaymentMethods: PropTypes.bool,
+
+    /** Is resize event happens on the web/desktop? */
+    isResizeListen: PropTypes.bool,


Not a blocker: change the prop name to listenResize?

rushatgabhane · 2022-02-07T10:51:55Z

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

@@ -178,6 +169,9 @@ class PaymentsPage extends React.Component {
     * Hide the add payment modal
     */
    hideAddPaymentMenu() {
+        if (this.props.isResizeListen) {
+            window.removeEventListener('resize', null);


Remove eventListener in hideDefaultDeleteMenu() too.

rushatgabhane · 2022-02-07T10:52:59Z

src/components/KYCWall/index.js

+KYCWall.propTypes = propTypes;
+KYCWall.defaultProps = defaultProps;
+KYCWall.displayName = 'KYCWall';
+export default KYCWall;


New line above export please

phivh · 2022-02-07T17:02:32Z

@rushatgabhane updated!

rushatgabhane

@deetergp LGTM and tests well on all platforms.

marcaaron · 2022-02-07T21:20:45Z

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

+import BasePaymentsPage from './BasePaymentsPage';
+
+const PaymentsPage = () => (
+    <BasePaymentsPage listenResize />


Not going to leave a full review here, but please see this draft PR which explains proper usage of boolean props to enable or disable features...

#7611

@rushatgabhane please 👀 and thank you 🙇

And applied this would be changed to something like shouldListenForResize.

Agreed. Let's change it.

Thanks, I agree!

deetergp

Overall looks good. Just had couple minor changes.

deetergp · 2022-02-07T22:30:17Z

src/components/KYCWall/BaseKYCWall.js

+     *
+     * @param {Object} position
+     */
+


Remove line break

deetergp · 2022-02-07T23:07:58Z

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

+import BasePaymentsPage from './BasePaymentsPage';
+
+const PaymentsPage = () => (
+    <BasePaymentsPage listenResize />


Agreed. Let's change it.

phivh · 2022-02-08T07:00:11Z

@deetergp @rushatgabhane updated!

deetergp

Looks good. Thanks for the changes.

phivh · 2022-02-09T10:46:46Z

Looks good. Thanks for the changes.

Should we merge it? It might require merging by core team.

luacmartins · 2022-02-09T13:14:12Z

@deetergp feel free to merge this as I don't think neither Marc nor I will leave a full review.

deetergp · 2022-02-09T16:57:10Z

Thanks, @luacmartins, but I think @rushatgabhane may want to give it another look. If not, I'll merge.

rushatgabhane

@deetergp LGTM! 🎉️

deetergp · 2022-02-09T17:41:19Z

No good deed goes unpunished! We've got two approvals, but now we've got a merge conflict. @phivh Once you fix the conflict, I'll re-approve and merge.

phivh · 2022-02-10T13:38:35Z

@deetergp I've just updated. Could you please take a look if anything is ok?

OSBotify · 2022-02-11T22:35:34Z

✋ 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-02-15T19:32:34Z

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

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

marcaaron · 2022-02-16T00:11:55Z

src/components/KYCWall/BaseKYCWall.js

+            let clickedElementLocation = getClickedElementLocation(event.nativeEvent);
+            let position = this.getAnchorPosition(clickedElementLocation);
+            if (this.props.shouldListenForResize) {
+                window.addEventListener('resize', () => {


Oh, hmm, this is gonna bind a new event listener each time. That can have unintended consequences. The best practice would be to set an event listener once on component init and then clean it up on unmount...

Here's an example of what that looks like using a class component

https://reactjs.org/docs/hooks-effect.html#example-using-classes-1

cc @rushatgabhane @deetergp

hmm, we already added a remove listener when the modal close. The scenario is add listener when the modal appears and remove listener when the modal closes.

Please fix this. Here are the instructions...

add event listeners in componentDidMount()

remove event listeners in componentWillUnmount()

bind the callbacks they use i.e. don't use an arrow function

Generally speaking even if we are focusing on the web side for consistency we should use the Dimensions utility. React native web is able to use this.

https://reactnative.dev/docs/0.66/dimensions#addeventlistener

@phivh right now, we're adding a event listener each time the window is resized.
So yeah, let's create a follow-up PR to fix this. What do you think?

ok, let me take into this one

@marcaaron @deetergp @rushatgabhane created follow-up PR for this one. Please take a look when you guys have a chance.
#7847

OSBotify · 2022-02-23T21:21:53Z

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

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

phivh added 2 commits February 4, 2022 22:41

Fix add payment position dynamic

056ae84

Merge branch 'main' into fix/add-payment-dynamic

bf98fb6

phivh requested a review from a team as a code owner February 4, 2022 16:04

MelvinBot requested review from deetergp and rushatgabhane and removed request for a team February 4, 2022 16:04

rushatgabhane suggested changes Feb 4, 2022

View reviewed changes

cleanup

789c3ec

rushatgabhane reviewed Feb 6, 2022

View reviewed changes

rushatgabhane suggested changes Feb 7, 2022

View reviewed changes

cleanup

9de73c1

phivh requested a review from rushatgabhane February 7, 2022 17:02

rushatgabhane previously approved these changes Feb 7, 2022

View reviewed changes

marcaaron reviewed Feb 7, 2022

View reviewed changes

deetergp requested changes Feb 7, 2022

View reviewed changes

Change usage of boolean prop name

2298ebc

phivh dismissed rushatgabhane’s stale review via 2298ebc February 8, 2022 06:59

deetergp approved these changes Feb 8, 2022

View reviewed changes

rushatgabhane approved these changes Feb 9, 2022

View reviewed changes

Merge branch 'main' into fix/add-payment-dynamic

62f1a74

deetergp merged commit 1898d81 into Expensify:main Feb 11, 2022

OSBotify mentioned this pull request Feb 15, 2022

Deploy Checklist: New Expensify 2022-02-15 #7759

Closed

85 tasks

marcaaron reviewed Feb 16, 2022

View reviewed changes

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

Fix: Add payment dynamic #7566

Fix: Add payment dynamic #7566

Conversation

phivh commented Feb 4, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rushatgabhane left a comment

Choose a reason for hiding this comment

phivh commented Feb 6, 2022

rushatgabhane Feb 6, 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

luacmartins Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane left a comment

Choose a reason for hiding this comment

rushatgabhane Feb 7, 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

rushatgabhane Feb 7, 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

Choose a reason for hiding this comment

phivh commented Feb 7, 2022

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

Choose a reason for hiding this comment

deetergp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phivh commented Feb 8, 2022

deetergp left a comment

Choose a reason for hiding this comment

phivh commented Feb 9, 2022

luacmartins commented Feb 9, 2022

deetergp commented Feb 9, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

deetergp commented Feb 9, 2022

phivh commented Feb 10, 2022

OSBotify commented Feb 11, 2022

OSBotify commented Feb 15, 2022

Choose a reason for hiding this comment

phivh Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

marcaaron Feb 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

OSBotify commented Feb 23, 2022

rushatgabhane Feb 6, 2022 •

edited

Loading

luacmartins Feb 7, 2022 •

edited

Loading

rushatgabhane Feb 7, 2022 •

edited

Loading

rushatgabhane Feb 7, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

phivh Feb 16, 2022 •

edited

Loading

marcaaron Feb 16, 2022 •

edited

Loading