[PAID] [$250] Search - Saved search area is partially overlapped by url field

[lanitochka17]

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: 9.0.47
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Open Search tab
3. Open any saved search
4. Click 3-dot menu next to it > Rename
5. Enter an emoji or name with X:Y format and save it
6. Tap "v" at the top of a Search page to open the menu
7. Observe the saved search area.

Expected Result:

Saved search area is fully visible

Actual Result:

Saved search area is partially covered by the url field when opened after renaming it or right after saving the search

Workaround:

Unknown

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

Add any screenshot/video evidence

Bug6618621_1727541270278.Rename_search_mWeb.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021846258880159780544
• Upwork Job ID: 1846258880159780544
• Last Price Increase: 2024-10-15
• Automatic offers:
• ikevin127 | Reviewer | 104486937
• mkzie2 | Contributor | 104486939

Issue Owner

Current Issue Owner: @strepanier03

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

[lanitochka17]

@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

[mkzie2]

Edited by proposal-police: This proposal was edited at 2024-10-17 16:12:41 UTC.

Proposal

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

Saved search area is partially covered by the url field when opened after renaming it or right after saving the search

What is the root cause of that problem?

The issue happens if we exceed the maxHeight of the ScrollView in PopoverMenu. The wrap component of the PopoverMenu modal has paddingVertical is 16px so the item inside ScrollView here can be overlapped

App/src/components/PopoverMenu.tsx

Line 268 in 39393b7

<ScrollView style={isSmallScreenWidth ? {maxHeight: windowHeight - 250} : styles.createMenuContainer}>

App/src/pages/Search/SearchTypeMenuNarrow.tsx

Lines 193 to 197 in 39393b7

	<PopoverMenu
	menuItems={allMenuItems as PopoverMenuItem[]}
	isVisible={isPopoverVisible}
	anchorPosition={styles.createMenuPositionSidebar(windowHeight)}
	onClose={closeMenu}

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

For this case, we should move the padding-bottom and padding-top from the wrap component of PopoverMenu to the ScrollView.

App/src/pages/Search/SearchTypeMenuNarrow.tsx

Lines 193 to 197 in 39393b7

	<PopoverMenu
	menuItems={allMenuItems as PopoverMenuItem[]}
	isVisible={isPopoverVisible}
	anchorPosition={styles.createMenuPositionSidebar(windowHeight)}
	onClose={closeMenu}

We will pass innerContainerStyle as styles.pv0 and scrollContainerStyle as styles.pv4 to PopoverMenu here

App/src/pages/Search/SearchTypeMenuNarrow.tsx

Line 193 in 39393b7

<PopoverMenu

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

@strepanier03 Huh... This is 4 days overdue. Who can take care of this?

[strepanier03]

Working on this now.

[strepanier03]

I don't have an iOS device to repro this on, but I think this is a very minor visual issue.

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

♻️ Reviewing today...

[ikevin127]

@strepanier03 I tried reproducing this on iOS Simulator and had no luck.

I reviewed a PR recently (#50244) which fixed the scrolling of the list on large layout devices where it wasn't working, and I think it's expected that when opening the popover menu, the list scroll is at the top when the layout has scrolling space expanded due to one or multiple saved search items, depending on device height.

Therefore I propose to close this as a non-issue. Otherwise I'd need more information on the device this was reproduced on and more detailed steps on how exactly - if I can reproduce then I'll also be able to test whether the existing proposal fixes the issue.

[mkzie2]

@ikevin127 You can try to save many saved searches and open the first saved search. Besides, I think the only problem here is that we have a space at the bottom.

[ikevin127]

Thanks for the info! I noticed the space at the bottom on my side as well even though it was not going over any of the saved search options.

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

I think there might be a mistake regarding the platforms on which the issue is occuring as the ✅ iOS: Native is checked but all videos where this was reproduced are on ✅ iOS: mWeb Safari from what I can see.

Before removing the white space from the bottom as suggested by the proposal, I want to ask whether the white space at the bottom was put there on purpose. If nobody knows I guess we can move forward with removing it.

This is how things look on my side (dark theme) and with Safari tab bar both on the bottom and on the top:

Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2024-10-16.at.14.34.37.mp4

cc @luacmartins @Kicu @Expensify/design Tagging all which either worked on SearchFiltersNarrow.tsx or might want have a say in whether we want that bottom space removed or anything else changed when it comes to this issue's Expected result.

Note

My take

I think that the bottom spacing referenced in #50592 (comment) is there for a reason as it matches the popover's side / top spacing (see below) and back to this issue: when we have multiple saved search items in the list or even if there's just one and the height of the device is small -> it's expected we would have overlapping while still being able to scroll -> therefore not an issue.

It would only be an issue if the overlapping would not allow scrolling and the option item would be cut by that spacing without being able to scroll to see it fully.

But maybe some of you might see this from a different perspective, hence I tagged you and opened this up for discussion.

Top / Bottom Spacing	Side Spacing

[shawnborton]

I think the bottom space is fine to be there, but it should scroll with the entire content of the modal. So basically we want these kinds of modals to have 16px padding on the top and bottom, but that padding should be considered as part of the scrollable area that is kept within the overall modal height, if that makes sense.

[mkzie2]

@shawnborton I think we should only move the padding bottom to the scrollable area. Moving the padding-top to the scrollable area the content will be close to the top edge when scrolling.

[shawnborton]

I don't think I agree with you, I think both top and bottom should be in the scrollable area. But let's see some videos to compare :)

[mkzie2]

@shawnborton Here is the video

• Only the bottom padding is in the scrollable area

Screen.Recording.2024-10-17.at.19.39.56.mov

• Both top and bottom padding are in the scrollable area

Screen.Recording.2024-10-17.at.19.39.31.mov

[shawnborton]

I think the second video (both top and bottom can scroll) looks much better to me. Curious what the rest of the @Expensify/design thinks though!

[dannymcclain]

I agree—I think that feels more natural/expected to me.

[mkzie2]

Updated proposal.

@ikevin127 The design is confirmed. Please help to take a look at my proposal again.

[ikevin127]

Given this was confirmed with the design team, @mkzie2's updated proposal looks good to me. The RCA is correct and the proposed solution addresses the issue as required.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[dubielzyk-expensify]

100% with the second video 👍

[ikevin127]

cc @luacmartins for assignment here based on proposal review from #50592 (comment).

Edit: A heartfelt thank you!

[melvin-bot]

📣 @ikevin127 🎉 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]

📣 @mkzie2 🎉 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 📖

[ikevin127]

@mkzie2 Please let us know when we can expect a PR to be ready for review 🧑‍💻

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

• fix: saved search is overlapped by bottom padding #51210

If no regressions arise, payment will be issued on 2024-10-30. 🎊

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

• @ikevin127 requires payment automatic offer (Reviewer)
• @mkzie2 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:

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

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: [Search v1] Add SearchFilters component #40419.
• [@ikevin127] 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/40419/files#r1819876695.
• [@ikevin127] 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.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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 Proposal

1. Go to Search.

2. Create several saved search items if none (to make the popover scrollable).

3. Press the header's dropdown button.

4. Verify there's top padding in the popover.

5. Scroll the list.

6. Verify there's NO top AND bottom padding while scrolling.

7. Scroll to the bottom.

8. Verify there's bottom padding.

   In other words, the padding is scrolled with the list instead of being fixed at the top or bottom.

Do we agree 👍 or 👎.

[strepanier03]

Reg test submitted. Handling payments now.

[strepanier03]

Payment Summary

• $250 - @ikevin127 via Upwork (Reviewer)
• $250 - @mkzie2 via Upwork (Contributor)