[PAID] [$250] Android - Wallet - BA number in two lines is cut off at the bottom

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.18-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4836144
Email or phone of affected tester (no customers): fischer9966+080824aa@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition: Android phone with horizontal screen resolution 1080 px - Eq Google Pixel 5

1. Open Android App
2. Login with any account
3. Go to Settings > Wallet
4. Add 2 bank account - 0000 and 1111 and set account 0000 as Default. Account number should occupy 2 lines.
5. Check that account is displayed correctly

Expected Result:

Bank account occupying two lines is displayed correctly, all numbers are fully visible

Actual Result:

Bank account occupying two lines is cut off at the bottom

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6565304_1723099206614.Android-BA-Number-Cut-Bottom.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0127ebfac638cf5ec5
• Upwork Job ID: 1823557759281995585
• Last Price Increase: 2024-08-14
• Automatic offers:
• paultsimura | Reviewer | 103603910
• FitseTLT | Contributor | 103603915

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot]

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

We think that this bug might be related to #vip-vsb

[IuliiaHerets]

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-08-08 19:43:42 UTC.

Proposal

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

Wallet - BA number in two lines is cut off at the bottom

What is the root cause of that problem?

We have set a fixed height of the menu item here

App/src/pages/settings/Wallet/PaymentMethodList.tsx

Lines 354 to 355 in d6f64ee

	wrapperStyle={[styles.paymentMethod, listItemStyle]}
	iconRight={item.iconRight}

App/src/styles/index.ts

Lines 3785 to 3786 in d6f64ee

	height: variables.optionRowHeight,
	},

but when the number of line of description is 2 the current height is not enough and it gets cut

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

We have many options

1. we can increase the menu item height according to suggestions from design team
2. omit setting fixed height style for the menu item so that it is flexible based on its content
3. we can decrease the padding so that text will not get to the next line when the badge is present
4. We can also set numberOfLinesDescription to 1 b/c the default is 2

What alternative solutions did you explore? (Optional)

[melvin-bot]

@strepanier03 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0127ebfac638cf5ec5

[melvin-bot]

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

[paultsimura]

@FitseTLT did you manage to reproduce the bug? If yes – which device did you use in the simulator?

[FitseTLT]

@FitseTLT did you manage to reproduce the bug? If yes – which device did you use in the simulator?

I Did manage to reproduce it but it's not the device type that matters but the Accessibility > Text Size setting (You have to increase it a bit)

[paultsimura]

Thank you @FitseTLT I managed to reproduce as well. From the list of your suggested solutions, which one do you prefer and why?

[FitseTLT]

Thank you @FitseTLT I managed to reproduce as well. From the list of your suggested solutions, which one do you prefer and why?

Obviously the correct thing to do would be to tweak the height style.

1. It would be best if we omit menu item height style so that height will be flexibly determined by its content. Do you know any downsides to this??
2. Otherwise, we can increase the height, based on maximum text sizes. For instance, the description text size in this case ranges from 13 to 19 px based on the accessibility setting
   

   App/src/styles/variables.ts

   Lines 55 to 56 in d4d5a25

   	fontSizeLabel: getValueUsingPixelRatio(13, 19),
   	fontSizeNormal: getValueUsingPixelRatio(15, 21),

   
   and for the title
   

   App/src/styles/variables.ts

   Lines 56 to 57 in d4d5a25

   	fontSizeNormal: getValueUsingPixelRatio(15, 21),
   	fontSizeMedium: getValueUsingPixelRatio(16, 22),

   
   So assuming the current size is more compatible to the min sizes we can increase the height of the menu item to fit for the 6px discrepancy per one line of text.

[paultsimura]

Ok, I like your proposal so far @FitseTLT.

But we need an opinion of @Expensify/design here.

We plan to change the paymentMethod style to use minHeight instead of height here:

App/src/styles/index.ts

Line 3780 in 5d09846

height: variables.optionRowHeight,

This will fix the menu items overflow (before/after):

mWeb:

Android:

---

Question: are there any drawbacks to it that I might be missing? And does the vertical centering look ok to you on the last screenshot?

[shawnborton]

Hmm in the screenshots above, why aren't we truncating the "Account ending in..." line after 1 line as well? Can we first start there and see how it looks?

[paultsimura]

@shawnborton because that's what OP stated as expected behavior😅
Should we truncate text instead?

[shawnborton]

I think it makes more sense to truncate after 1 line here given the width constraints personally. Let's see if @Expensify/design agrees though!

[dannymcclain]

I think it makes more sense to truncate after 1 line here given the width constraints personally.

I'm a little torn, because if we truncate we lose the only valuable info from that line of text (the last four digits). Personally I think it might make more sense to ditch the word Account in the line Account ending in 0000. The title of the card is Bank accounts so I'm wondering if

Plaid Checking
Ending in 0000

is enough info for the user to understand what we're talking about here. What do you think about that?

[shawnborton]

Big fan of that, I think that will save us a lot of room!

[paultsimura]

Great, thanks for the discussion.
Let's proceed with @FitseTLT based on the proposal. We can adjust the copy in a PR as well as see if we need any further alterations such as truncating.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @roryabraham, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

@strepanier03, @paultsimura, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[paultsimura]

This was deployed to production in v9.0.26-6.
Payment is due 2024-09-06.

[strepanier03]

Looks like automation failed so I updated the title and set to Daily so I don't lose track of this one.

[melvin-bot]

@strepanier03 @paultsimura @FitseTLT @roryabraham this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[paultsimura]

• The PR that introduced the bug has been identified. Link to the PR: Add New payments page #3727 – not really an offending PR – just an overlooked design piece.
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/3727/files#r1746006195
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug: No
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

[strepanier03]

Payment summary

• $250 - @FitseTLT via Upwork (paid)
• $250 - @paultsimura via Upwork (paid)

Thanks everyone!