Open lanitochka17 opened 1 week ago
melvin-bot[bot]
commented
1 week ago 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
commented
1 week ago @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
commented
1 week ago Edited by proposal-police: This proposal was edited at 2024-10-17 16:12:41 UTC.
Saved search area is partially covered by the url field when opened after renaming it or right after saving the search
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
For this case, we should move the padding-bottom and padding-top from the wrap component of PopoverMenu to the ScrollView.
We will pass innerContainerStyle as styles.pv0 and scrollContainerStyle as styles.pv4 to PopoverMenu here
NA
melvin-bot[bot]
commented
6 days ago @strepanier03 Huh... This is 4 days overdue. Who can take care of this?
strepanier03
commented
6 days ago Working on this now.
strepanier03
commented
6 days ago I don't have an iOS device to repro this on, but I think this is a very minor visual issue.
melvin-bot[bot]
commented
6 days ago Job added to Upwork: https://www.upwork.com/jobs/~021846258880159780544
melvin-bot[bot]
commented
6 days ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)
ikevin127
commented
6 days ago ♻️ Reviewing today...
ikevin127
commented
6 days ago @strepanier03 I tried reproducing this on iOS Simulator and had no luck.
I reviewed a PR recently (https://github.com/Expensify/App/pull/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
commented
5 days ago @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
commented
5 days ago 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:
https://github.com/user-attachments/assets/284ce84c-0528-458f-b59b-e6ebf6f0c8cd
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 https://github.com/Expensify/App/issues/50592#issuecomment-2415674664 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
commented
4 days ago 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
commented
4 days ago @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
commented
4 days ago 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
commented
4 days ago @shawnborton Here is the video
https://github.com/user-attachments/assets/2175da75-e09f-4f1d-80bb-112d2000cf61
https://github.com/user-attachments/assets/dcb37670-bf67-49b9-95fb-e8eab7571eae
shawnborton
commented
4 days ago 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
commented
4 days ago I agree—I think that feels more natural/expected to me.
mkzie2
commented
4 days ago Updated proposal.
@ikevin127 The design is confirmed. Please help to take a look at my proposal again.
ikevin127
commented
4 days ago 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[bot]
commented
4 days ago Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
dubielzyk-expensify
commented
4 days ago 100% with the second video 👍
ikevin127
commented
3 days ago cc @luacmartins for assignment here based on proposal review from https://github.com/Expensify/App/issues/50592#issuecomment-2420474775.
Edit: A heartfelt thank you!
melvin-bot[bot]
commented
3 days ago 📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
3 days ago 📣 @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
commented
2 days ago @mkzie2 Please let us know when we can expect a PR to be ready for review 🧑💻
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.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:
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?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/de3d2df7-2069-472d-a1f8-16dc837ae62a
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @ikevin127