[HOLD for payment 2023-09-27] [HOLD for #26747] [$1000] Request money - Amount is partially covered by top selection menu

[lanitochka17]

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

---

issue found when executing PR #23698

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in with an expensifail account
3. Tap the green plus button
4. Tap on "Request money"

Expected Result:

Amount should be fully visible

Actual Result:

The amount is partially covered by top selection menu (because of the banner on top of the screen)

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.54.5

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6166123_Krst4127_1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0152c32e49ad0a8a1b
• Upwork Job ID: 1691855787116150784
• Last Price Increase: 2023-08-16
• Automatic offers:
• tienifr | Contributor | 26249176

[melvin-bot]

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[himanshuragi456]

what should be the fix?
either we'll have to reduce some element sizes or we can remove options on the top

[allroundexperts]

Proposal

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

Request money - Amount is partially covered by top selection menu

What is the root cause of that problem?

Space is not enough for all the things to render. We're not wrapping our MoneyRequestForm inside a ScrollView and are relying on just the screen height to display everything.

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

We need to conditionally replace the Fragment defined here with the following, only if isEditing is false.

<ScrollView contentContainerStyle={[styles.flexGrow1, styles.flexColumn]}>

Result

Screen.Recording.2023-08-16.at.2.37.20.AM.mov

What alternative solutions did you explore? (Optional)

None

[tienifr]

Proposal

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

Request money - Amount is partially covered by top selection menu

What is the root cause of that problem?

The height of each button is too big (minHeight: 52px), so in the small screen we don't have enough space to show the amount

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

We should calculate the height for the button base on the percentage. In order to do that we can lean on heightPercentageToDP from react-native-responsive-screen

// 812 is the default screen height
// value is the height that we want to be in the default screen
export const responsiveByHeight = (value: number, referenceScreenHeight: number = 812) => {
    return heightPercentageToDP((value / referenceScreenHeight) * 100);
};

In

App/src/components/BigNumberPad.js

Line 72 in 0bdf183

<Button

we can add these lines

innerStyles={[{
    minHeight:responsiveByHeight(variables.componentSizeLarge) // we can change the size here
}]}

Note: this approach won't change the heigh when we change the screen height dynamically, but I think it's fine because we can only use BigNumberPad on mweb or native, and we don't have any ways to change the screen height dynamically.

If we still want to do that, we can build the heightPercentageToDP by ourself. The idea is the same as heightPercentageToDP from react-native-responsive-screen, it is to calculate the height base on the percentage and the screen height (get from withWindowDimensions)

What alternative solutions did you explore? (Optional)

None

Result

Screen.Recording.2023-08-17.at.15.20.11.mov

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0152c32e49ad0a8a1b

[melvin-bot]

Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[parasharrajat]

@lanitochka17 What is the device you are testing on?

[allroundexperts]

For me, this happens on iPhone SE only when the Open in New Expensify App banner shows up.

[parasharrajat]

I don't like both of the solutions. Both do not serve the best UX. let's think about this problem in a different way. The whole app looks a little bit zoomed in due to the low dpi of the phone. Is there a solution to handle low dpi phones without changing the layout of pages?

IMO, UI should handle dpis otherwise same elements will look smaller on high DPI and bigger on low DPI. I hope you got my point...

[allroundexperts]

I get what you're saying here but I think even then we can get into the same situation on very small heighted screens. In those edge cases, showing a scrollbar won't effect the UX for other users.

[parasharrajat]

So you are saying that it makes sense that the field where you are entering the amount that will either be sent or received, is hidden while you are typing... It will make me nervous for sure. I will not prefer sending money with that app.

[allroundexperts]

It won't be hidden unless you're using a device with a height < 200px. For the device mentioned in this issue, only the submit button would be hidden (You would be able to type and see the amount). If you are on a super rare device with a height of less than 200, then you really should be ready to compromise.

[lanitochka17]

@parasharrajat Apple iPhone SE (2020) / Safari

[tienifr]

IMO, UI should handle dpis otherwise same elements will look smaller on high DPI and bigger on low DPI. I hope you got my point...

In order to do that we can lean on heightPercentageToDP function from react-native-responsive-screen. What do you think about this solution? @parasharrajat

[parasharrajat]

Does this work on web as well?

[tienifr]

Yes, It works well on web @parasharrajat

[tienifr]

Here's the result

[parasharrajat]

can you share a proposal on this? As this is a UI change, please leave a video as well indicate the changes to the overall app.

[tienifr]

@parasharrajat Here you are #24603 (comment). Pls tell me what's your thoughts

[parasharrajat]

Nope, that's not what I was thinking. It changes our aesthetics and design system. The UI does not look the same in contrast to what I said.

An optimal solution would be that the whole app looks the same. Every element on that page adjusts in a way that app looks the same. I am afraid that this won't be a small change. To do this, we have to change everything and make those dpi independent. Font sizes, heights, widths, etc.

Let's take an example of the lib you posted.

Look closely the feel and look of the app do not changes on all devices.

[allroundexperts]

@parasharrajat I think this is more of a feature request which is beyond the scope of this bug ticket. We might want to consult @shawnborton about his thoughts on this matter.

[tienifr]

@parasharrajat PR is up here #26747.

[stephanieelliott]

Removing Hold for payment, will update once #26747 is merged.

[stephanieelliott]

#26747 still open

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @tienifr got assigned: 2023-08-21 20:50:44 Z
• when the PR got merged: 2023-09-18 06:12:22 UTC
• days elapsed: 19

On to the next one 🚀

[stitesExpensify]

FYI ^that is incorrect. I did not review this on time because we were on a merge hold, so this is eligible for a bonus

[tienifr]

Thanks @stitesExpensify but that one is a regression fix.

[stitesExpensify]

Ah 🤦 thanks for the clarification

[parasharrajat]

We can remove the hold from title.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.71-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Regression - Big number pad button size is not justified on small screens #26747

If no regressions arise, payment will be issued on 2023-09-27. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @parasharrajat does not require payment (Eligible for Manual Requests)
• @tienifr requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] 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:
• [@parasharrajat] 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:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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.
• [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[parasharrajat]

@stephanieelliott There was a regression from this main PR which was reported https://expensify.slack.com/archives/C049HHMV9SM/p1693367178391569 and we fixed it in #26747.

Can you please add a reporting bonus for @wildan-m for this report as well?

[parasharrajat]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: This is a new change.
• [@parasharrajat] 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: N/A
• [@parasharrajat] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1695202262248269
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

1. Login with any account on small screen devices (iPhone SE)
2. Press FAB >> Request money.
3. Select the Manual Tab.
4. Verify that the amount input is fully visible and number pad is fully visible.

Do you agree 👍 or 👎 ?

[stephanieelliott]

Summarizing payment on this issue:

• Issue reporter: @wildan-m $250 via Upwork (offer extended in Upwork)
• Contributor: @tienifr $1000 (paid in Upwork)
• Contributor+: @parasharrajat $1000 via Newdot

Upwork job is here, no bonus

[wildan-m]

Hi @stephanieelliott thanks for the offer. I wanted to confirm the amount. My bug report is valid but it's not exported to github yet. If we refer to this link, how much I should receive? $50 or $250? (Of course I'll be happy with the last one 😁)

Note: my bug report posted one day before bonus recalibration published.

[stephanieelliott]

@wildan-m I'm basing off the date this GH issue was created, so it would be $250. Thanks for checking!

[stephanieelliott]

All paid!

[wildan-m]

@stephanieelliott Thanks, you are the best! 😺

[parasharrajat]

Payment requested as per #24603 (comment)

[JmillsExpensify]

$1,000 payment approved for @parasharrajat based on BZ summary.