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

liyamahendra · 2022-06-23T20:04:32Z

Details

The payment options were overlapping when they are shown after clicking on the 'Add Payment Method' button.

Fixed Issues

$ #9283

Tests

Click on the 'Add Payment Method' button2.
Verify that the payment method options pop-over menu is shown below the 'Add Payment Method' button3.
Also verify that the payment method options pop-over menu resizes / displaces appropriately when the browser is resized.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Click on the 'Add Payment Method' button2.
Verify that the payment method options pop-over menu is shown below the 'Add Payment Method' button3.
Also verify that the payment method options pop-over menu resizes / displaces appropriately when the browser is resized.

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron · 2022-06-23T22:24:59Z

Can you please update this PR so that it has a more recognizable title? Thanks.

liyamahendra · 2022-06-24T03:38:53Z

@marcaaron Update the title of the PR.

src/libs/getClickedTargetLocation/index.js

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

mananjadhav

Just one question on retaining the existing function and also fixing the issue for KYCWall.

liyamahendra · 2022-06-24T06:13:47Z

@mananjadhav I did left a comment requesting details of BaseKYCWall and I can see if we repurpose getClickedElementLocation. Would you share some insights please?

Would you please share insights into how can I test BaseKYCWall. Then, I'll make sure I update getClickedElementLocation as required.

mananjadhav · 2022-06-24T06:29:55Z

@liyamahendra I'll also have to check how that flow works.

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

Lines 262 to 273 in 2595343

    
           {Permissions.canUseWallet(this.props.betas) && ( 
        
               <> 
        
                   <View style={[styles.mv5]}> 
        
                       <CurrentWalletBalance /> 
        
                   </View> 
        
                   {this.props.userWallet.currentBalance > 0 && ( 
        
                       <KYCWall 
        
                           onSuccessfulKYC={this.navigateToTransferBalancePage} 
        
                           enablePaymentsRoute={ROUTES.SETTINGS_ENABLE_PAYMENTS} 
        
                           addBankAccountRoute={ROUTES.SETTINGS_ADD_BANK_ACCOUNT} 
        
                           addDebitCardRoute={ROUTES.SETTINGS_ADD_DEBIT_CARD} 
        
                           popoverPlacement="bottom"

A quick search leads me to this code where you can force the conditions on L262 and L267 to be true. I hope this helps.

mananjadhav · 2022-06-28T07:56:45Z

@liyamahendra Any updates for me on the KYCWall page? Do you still need help with the same?

liyamahendra · 2022-06-28T08:01:06Z

Ah, apologies @mananjadhav , I assumed you'll check the flow works for KYCWall and share the details.

Allow me to review this from my end and see if I can figure this out. If not, I'll ask specific questions please.

liyamahendra · 2022-06-28T08:15:13Z

@mananjadhav I checked the code. Would you mind sharing details as to how can I test / see the KYCWall screen rendered in the web app? This will allow me to validate and do the change.

Co-authored-by: Manan <[email protected]>

mananjadhav · 2022-06-28T09:25:29Z

as to how can I test / see the KYCWall screen rendered in the web app?

I don't know the flow either. I can spend some time tomorrow and get you the flow. In my opinion to just verify if the problem exists, we could remove those conditions and force view the page may be by visiting the route?

liyamahendra · 2022-06-28T14:30:51Z

@mananjadhav I couldn't infer the actual page and how to visit it. Please advise.

mananjadhav · 2022-06-30T19:21:58Z

I'll check this by tomorrow and share the Test steps with you.

mananjadhav · 2022-07-03T18:19:52Z

@liyamahendra The steps are almost through the force code changes that I asked for (I don't have the betas access)

In BasePaymentPage force true the conditions the following conditions on L262 and L267.

{(true || Permissions.canUseWallet(this.props.betas)) && (

and

{(true || this.props.userWallet.currentBalance > 0) && (

Go to the Profile Picture -> RHN (right side bar) -> Payments
You'll see a Transfer Balance button
Hover over it, to see the Payment options.

These steps should help you for the KYCWall.js.

liyamahendra · 2022-07-05T05:38:44Z

@mananjadhav thank you for the steps. However, even after doing the changes you suggested, I don't see the Transfer Balance button. See the attached screencast.

transfer-balance.mp4

liyamahendra · 2022-07-05T05:45:24Z

@mananjadhav I removed the conditions altogether and was able to see the Transfer Balance Button. While testing it, I don't see the issue with it. See the screencast below.

transfer-balance.mp4

Can you advise please?

mananjadhav · 2022-07-05T09:49:54Z

@liyamahendra Goal is to ensure that when we're replacing the function (because it works on the similar logic), lets ensure that we use your function for this component as well.

liyamahendra · 2022-07-05T10:36:39Z

Got it @mananjadhav - since we agreed to remove the former function, I'll test out how this behaves with my function and comment back

…entLocation

liyamahendra · 2022-07-05T12:40:06Z

@mananjadhav have done the changes to KYCWall.

mananjadhav · 2022-07-12T06:04:23Z

@liyamahendra I reviewed and tested your changes, but we need to sync the main branch and I can take a look at it again.

Also while the changes are not different parts of the page, I feel we might get some conflicts if #9622 gets merged. I would also personally recommend testing the changes after pulling the latest changes.

Lastly, please complete the PR checklist. Please mark the items off the checklist even if they don't apply. Tests are mandatory for all Platforms (I can see Android/Native not marked and missing screenshot).

liyamahendra · 2022-07-12T15:56:37Z

Lastly, please complete the PR checklist. Please mark the items off the checklist even if they don't apply. Tests are mandatory for all Platforms (I can see Android/Native not marked and missing screenshot).

@mananjadhav I uploaded a screenshot for Android and completed the checklist. I also pulled from Main and tested this particular issue. Didn't encounter any conflicts.

mananjadhav

@marcaaron Review done

🎀 👀 🎀
C+ reviewed

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

marcaaron

Looks good, but I have a couple of questions that should be answered before we merge.

src/libs/getClickedTargetLocation/index.js

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

Co-authored-by: Marc Glasser <[email protected]>

parasharrajat · 2022-08-30T16:12:18Z

Bump. Are we waiting for something here? I have a PR where need changes of this PR.

mananjadhav · 2022-08-30T16:22:23Z

I am left with testing it once again. Will do that by today-tomorrow

mananjadhav · 2022-08-30T19:16:24Z

@marcaaron Review done.

🎀 👀 🎀
C+ reviewed

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

OSBotify · 2022-08-30T19:35:25Z

✋ 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-08-31T09:32:21Z

🚀 Deployed to staging by @marcaaron in version: 1.1.95-0 🚀

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

liyamahendra added 2 commits June 15, 2022 13:06

Solved overlapping of payment method options

fb5605f

Reused variables

0fb60bb

liyamahendra requested a review from a team as a code owner June 23, 2022 20:04

melvin-bot bot requested review from mananjadhav and marcaaron and removed request for a team June 23, 2022 20:04

resolved lint error

c074518

liyamahendra mentioned this pull request Jun 23, 2022

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

Closed

marcaaron assigned liyamahendra Jun 23, 2022

liyamahendra changed the title ~~9283~~ Solved the overlapping of payment options when clicking on 'Add Payment Method' Jun 24, 2022

mananjadhav reviewed Jun 24, 2022

View reviewed changes

src/libs/getClickedTargetLocation/index.js Show resolved Hide resolved

mananjadhav reviewed Jun 24, 2022

View reviewed changes

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Outdated Show resolved Hide resolved

mananjadhav suggested changes Jun 24, 2022

View reviewed changes

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

e7a74a5

Co-authored-by: Manan <[email protected]>

Using getClickedTargetLocation in KYCWall and removing getClickedElem…

95d8d9f

…entLocation

parasharrajat mentioned this pull request Jul 6, 2022

[$2000] iPad - UI Inconsistency of the popover menus - reported by @Santhosh-Sellavel #7375

Closed

Merge branch 'Expensify:main' into 9283

360f0ca

mananjadhav previously approved these changes Jul 12, 2022

View reviewed changes

marcaaron requested changes Jul 12, 2022

View reviewed changes

src/libs/getClickedTargetLocation/index.js Show resolved Hide resolved

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Show resolved Hide resolved

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Outdated Show resolved Hide resolved

removed 'addPaymentMethodButton: null' call

44f3695

liyamahendra dismissed mananjadhav’s stale review via 44f3695 July 16, 2022 04:56

marcaaron requested changes Aug 27, 2022

View reviewed changes

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Outdated Show resolved Hide resolved

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Outdated Show resolved Hide resolved

src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js Outdated Show resolved Hide resolved

liyamahendra and others added 3 commits August 27, 2022 13:51

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

52e99ba

Co-authored-by: Marc Glasser <[email protected]>

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

7990e9f

Co-authored-by: Marc Glasser <[email protected]>

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

151e27b

Co-authored-by: Marc Glasser <[email protected]>

marcaaron approved these changes Aug 30, 2022

View reviewed changes

marcaaron merged commit 8e07c62 into Expensify:main Aug 30, 2022

OSBotify mentioned this pull request Aug 31, 2022

Deploy Checklist: New Expensify 2022-08-31 #10712

Closed

15 tasks

liyamahendra deleted the 9283 branch August 31, 2022 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

liyamahendra commented Jun 23, 2022 •

edited

Loading

marcaaron commented Jun 23, 2022

liyamahendra commented Jun 24, 2022

mananjadhav left a comment

liyamahendra commented Jun 24, 2022

mananjadhav commented Jun 24, 2022 •

edited

Loading

mananjadhav commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

mananjadhav commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

mananjadhav commented Jun 30, 2022

mananjadhav commented Jul 3, 2022

liyamahendra commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

mananjadhav commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

mananjadhav commented Jul 12, 2022

liyamahendra commented Jul 12, 2022

mananjadhav left a comment •

edited

Loading

marcaaron left a comment

parasharrajat commented Aug 30, 2022

mananjadhav commented Aug 30, 2022

mananjadhav commented Aug 30, 2022

PR Reviewer Checklist

OSBotify commented Aug 30, 2022

OSBotify commented Aug 31, 2022

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

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

Conversation

liyamahendra commented Jun 23, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Jun 23, 2022

liyamahendra commented Jun 24, 2022

mananjadhav left a comment

Choose a reason for hiding this comment

liyamahendra commented Jun 24, 2022

mananjadhav commented Jun 24, 2022 • edited Loading

mananjadhav commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

mananjadhav commented Jun 28, 2022

liyamahendra commented Jun 28, 2022

mananjadhav commented Jun 30, 2022

mananjadhav commented Jul 3, 2022

liyamahendra commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

mananjadhav commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

liyamahendra commented Jul 5, 2022

mananjadhav commented Jul 12, 2022

liyamahendra commented Jul 12, 2022

mananjadhav left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

marcaaron left a comment

Choose a reason for hiding this comment

parasharrajat commented Aug 30, 2022

mananjadhav commented Aug 30, 2022

mananjadhav commented Aug 30, 2022

PR Reviewer Checklist

OSBotify commented Aug 30, 2022

OSBotify commented Aug 31, 2022

liyamahendra commented Jun 23, 2022 •

edited

Loading

mananjadhav commented Jun 24, 2022 •

edited

Loading

mananjadhav left a comment •

edited

Loading