App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Wallet - "Delete Bank Account" button is positioned incorrectly

Open kbecciv opened this issue 1 year ago • 18 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.36-0 Reproducible in staging?: y Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Open Desktop App or go to staging.new.expensify.com
  2. Login with any account
  3. Go to Settings > Wallet
  4. Tap 'Add bank account' > 'Personal Bank Account' and follow the flow
  5. Tap to added in step 4 bank account
  6. 'Delete' button appears

Expected Result:

  • Add the vertical three dots to the right-side of each bank account row to access the overflow menu
  • Clicking the row reveals the popover (bottom drawer on mobile) beneath the three dots icon, containing the two menu items to make default (if applicable) or delete the bank account
  • When the overflow menu is expanded, the three dots are green.
  • Choosing to delete the bank account closes the menu and a centred alert modal appears to confirm the action.

image image

  • Update the badge component so that it matches the new styles, and applied here for Default:
image

P.s ignore the wider redesign of this page you can see in these mocks, we're taking care of that elsewhere as a v2.

More discussion here in thread.

Actual Result:

The button 'Delete' is NOT aligned with the user interface elements and we use inconsistent patterns on this page.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [ ] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/51649e90-a74f-4150-99c0-d95ae9862791

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0180de917d0294db30
  • Upwork Job ID: 1754563551786045440
  • Last Price Increase: 2024-02-05

kbecciv avatar Feb 02 '24 21:02 kbecciv

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Feb 02 '24 21:02 github-actions[bot]

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Feb 02 '24 21:02 melvin-bot[bot]

I think we could demote this to NAB, but I'll leave it up to @francoisl

luacmartins avatar Feb 02 '24 21:02 luacmartins

Yeah I was thinking the same, I think it's a consequence of the element now being way wider than it used to be. We can check with the design team what they think but it currently doesn't look outrageous to me. One option would be to position the little popup based on the mouse position when you click.

  • Staging / dev with ideal nav

https://github.com/Expensify/App/assets/2229301/66c40480-fcb2-437c-9cb8-072fffbdf1a1


  • Production / old style

https://github.com/Expensify/App/assets/2229301/ee3e5825-aac6-45f5-b0a0-a99bb59a2c20


Slack discussion

francoisl avatar Feb 02 '24 21:02 francoisl

Proposal

Please re-state the problem that we are trying to solve in this issue.

Payment method popover positioned incorrectly in wallet screen

What is the root cause of that problem?

Earlier it used to be positioned in the RHN referencing the right side of the anchor ref. It should be aligned to the left side of anchor now. https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/settings/Wallet/WalletPage/WalletPage.js#L102

What changes do you think we should make in order to solve the problem?

  1. Instead anchorPositionRight make it anchorPositionLeft which we used to set for the popover. https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/settings/Wallet/WalletPage/WalletPage.js#L64

  2. Calculate the left position like anchorPositionLeft: position.x + variables.addBankAccountLeftSpacing,

  3. Set the value for left here instead of right left: anchorPosition.anchorPositionLeft, https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/settings/Wallet/WalletPage/WalletPage.js#L507

Result

https://github.com/Expensify/App/assets/14358475/2a90cc90-ef74-48fb-a9d5-eb093855da12

Pujan92 avatar Feb 04 '24 06:02 Pujan92

Job added to Upwork: https://www.upwork.com/jobs/~0180de917d0294db30

melvin-bot[bot] avatar Feb 05 '24 17:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 05 '24 17:02 melvin-bot[bot]

@Pujan92's proposal makes sense to me. It should now be aligned with other menus.

Opening the menu at cursor position is another option but that does not look good to me. e.g. > About page -> Right click on View open Jobs

~:ribbon: :eyes: :ribbon: C+ reviewe~

parasharrajat avatar Feb 05 '24 18:02 parasharrajat

Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Feb 05 '24 18:02 melvin-bot[bot]

👋 Hey gang! We chatted about this one in the thread here in #expensify-open-source. We'd like to make a couple of changes to the bank account rows while we're here fixing the alignment:

  • Add the vertical three dots to the right-side of each bank account row to access the overflow menu
  • Clicking the row reveals the popover (bottom drawer on mobile) beneath the three dots icon, containing the two menu items to make default (if applicable) or delete the bank account
  • When the overflow menu is expanded, the three dots are green.
  • Choosing to delete the bank account closes the menu and a centred alert modal appears to confirm the action.

image image

  • Update the badge component so that it matches the new styles, and applied here for Default:
image

P.s ignore the wider redesign of this page you can see in these mocks, we're taking care of that elsewhere as a v2.

trjExpensify avatar Feb 05 '24 22:02 trjExpensify

Left a teeeny tiny comment for ya in Figma about the location of the popover, but otherwise this looks good to me!

shawnborton avatar Feb 06 '24 01:02 shawnborton

Do we want to include a bullet about the default badge anywhere too, or is that a separate thing?

shawnborton avatar Feb 06 '24 01:02 shawnborton

Do we want to include a bullet about the default badge anywhere too, or is that a separate thing?

Ah, it exists already in prod.. but I guess Danny's design changes on it need to apply? Or wait, are those coming separately "as one" throughout /app somewhere? If not, then yup, I think we want to include that:

image

trjExpensify avatar Feb 06 '24 01:02 trjExpensify

This could be a good excuse to update the badge component so that it matches the new styles from Figma, but happy to squeeze it in elsewhere too.

shawnborton avatar Feb 06 '24 01:02 shawnborton

I think we can add the default tag easily. Proposed changed looks good.

@Pujan92 Please update your proposal.

parasharrajat avatar Feb 06 '24 06:02 parasharrajat

This could be a good excuse to update the badge component so that it matches the new styles from Figma,

Cool, let's include it. Updated that comment!

trjExpensify avatar Feb 06 '24 14:02 trjExpensify

@trjExpensify Please modify the Expected behaviour section for clarity.

parasharrajat avatar Feb 06 '24 19:02 parasharrajat

Yep, that's done!

trjExpensify avatar Feb 06 '24 23:02 trjExpensify

Hey! I’m Jakub Szymczak from Software Mansion, an expert agency, and I’d like to work on this issue! Was called out on slack thread and doing redesign of WalletPage so it would save me some conflicts if I do it along with redesign 😄

SzymczakJ avatar Feb 07 '24 14:02 SzymczakJ

Ah yeah, that's a good point. Cool, assigning this to you. Can you link the PR (when ready) to this issue and the broader redesign one? Cheers!

trjExpensify avatar Feb 07 '24 21:02 trjExpensify

This is completed @trjExpensify. let's close it.

parasharrajat avatar Feb 29 '24 22:02 parasharrajat

Bump @trjExpensify

parasharrajat avatar Mar 04 '24 12:03 parasharrajat

Done!

trjExpensify avatar Mar 04 '24 12:03 trjExpensify