App
App copied to clipboard
[$500] Wallet - "Delete Bank Account" button is positioned incorrectly
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:
- Open Desktop App or go to staging.new.expensify.com
- Login with any account
- Go to Settings > Wallet
- Tap 'Add bank account' > 'Personal Bank Account' and follow the flow
- Tap to added in step 4 bank account
- '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.
- Update the badge component so that it matches the new styles, and applied here for
Default
:
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
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~0180de917d0294db30
- Upwork Job ID: 1754563551786045440
- Last Price Increase: 2024-02-05
: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:
- Identify the pull request that introduced this issue and revert it.
- Find someone who can quickly fix the issue.
- Fix the issue yourself.
Triggered auto assignment to @francoisl (Engineering
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
I think we could demote this to NAB, but I'll leave it up to @francoisl
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
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?
-
Instead
anchorPositionRight
make itanchorPositionLeft
which we used to set for the popover. https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/settings/Wallet/WalletPage/WalletPage.js#L64 -
Calculate the left position like
anchorPositionLeft: position.x + variables.addBankAccountLeftSpacing,
-
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
Job added to Upwork: https://www.upwork.com/jobs/~0180de917d0294db30
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External
)
@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~
Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
👋 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.
- Update the badge component so that it matches the new styles, and applied here for
Default
:
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.
Left a teeeny tiny comment for ya in Figma about the location of the popover, but otherwise this looks good to me!
Do we want to include a bullet about the default badge anywhere too, or is that a separate thing?
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:
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.
I think we can add the default tag easily. Proposed changed looks good.
@Pujan92 Please update your proposal.
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 Please modify the Expected behaviour section for clarity.
Yep, that's done!
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 😄
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!
This is completed @trjExpensify. let's close it.
Bump @trjExpensify
Done!