[HOLD for payment 2024-06-24] [$250] The FAB or Search category selector are narrow

[mountiny]

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: v1.4.83-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: @mountiny
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1718283502781369

Action Performed:

Break down in numbered steps

1. Click on the + green button
2. Or go to Search page and select Expenses to choose different category to show

Expected Result:

Describe what you think should've happened

The modal show take the entire width of the screen

Actual Result:

Describe what actually happened

The modal is very narrow

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

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

Screenshots/Videos

RPReplay_Final1718283365.MP4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0163f71b7719038ed3
• Upwork Job ID: 1801238352430390753
• Last Price Increase: 2024-06-13
• Automatic offers:
• hoangzinh | Reviewer | 102723522
• bernhardoj | Contributor | 102723525

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0163f71b7719038ed3

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @johncschuster (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.

[github-actions]

👋 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.

[fedirjh]

This is a progression from #39520, it's caused by this extra view

App/src/components/Modal/BaseModal.tsx

Line 256 in f6de195

<View>

It can be fixed with the following code :

<View style={[isSmallScreenWidth && styles.w100]}>

[bernhardoj]

Proposal

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

The modal in mobile has a very narrow width.

What is the root cause of that problem?

In a recent focus trap PR, we added an extra plain View to the Modal children which doesn't have any style.

App/src/components/Modal/BaseModal.tsx

Lines 254 to 259 in f6de195

	>
	<FocusTrapForModal active={isVisible}>
	<View>
	<ModalContent onDismiss={handleDismissModal}>
	<View
	style={[styles.defaultModalContainer, modalPaddingStyles, modalContainerStyle, !isVisible && styles.pointerEventsNone]}

The correct width, spacing, etc. style of a modal is applied to the View inside ModalContent.

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

We can technically apply the same style to the extra View too, but instead of having 2 View, we can just have 1.

1. Remove the extra View
2. Move the FocusTrapForModal below the ModalContent

<ModalContent ...>
    <FocusTrapForModal active={isVisible}>
        <View style...>

[fedirjh]

@bernhardoj I guess this will break the focus trap, more context #39520 (comment)

cc @jnowakow

[bernhardoj]

It works fine as far as I tested

Screen.Recording.2024-06-13.at.21.36.27.mov

I updated my comment to a proposal in case we want to fix this externally

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[jnowakow]

@bernhardoj's proposal seems to be better option than applying style to this additional view. It's important to have View as a child of FocusTrapForModal but there's no problem in placing it inside ModalContent.

[dangrous]

That makes sense to me as well as long as it doesn't break the focus trap, which I defer to @jnowakow and @fedirjh to confirm. Seems like we're okay?

[fedirjh]

Looks good to me as well, it looks like the focus trap is working as expected. Let's proceed with @bernhardoj proposal.

[mountiny]

This was CPed and fixed

$250 to @bernhardoj and to @allroundexperts

[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.4.84-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] fix modal views are not fully shown #43706

If no regressions arise, payment will be issued on 2024-06-24. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

[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:

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

[melvin-bot]

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

• [CP Staging] fix modal views are not fully shown #43706

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

[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:

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

[johncschuster]

Discussing in Slack to make sure I'm understanding the correct payment date

[johncschuster]

Payment Summary:

Contributor+: @allroundexperts - $250 - paid via Manual request
Contributor: @bernhardoj - $250 - paid via Upwork - PAID

@allroundexperts, can you complete the BZ Checklist when you get a moment? Thank you!

[luacmartins]

I think the due date here is the 24th. Updated the title to reflect that.

[johncschuster]

Yep! Payment has already been issued via Upwork. We're just waiting on the BZ Checklist to be completed.

[johncschuster]

@allroundexperts can you complete the BZ Checklist above? Once finished, we can close this up.

[allroundexperts]

Checklist

1. Add focus trap #39520
2. The software mansion team was aware of this issue already here.
3. N/A
4. I think that the FAB and search category selector are being covered in other regression tests so a new one is not needed.

[johncschuster]

Thanks, @allroundexperts! Please go ahead and request payment if you haven't already!

[JmillsExpensify]

$250 approved for @allroundexperts