Android - 3-button navigation bar overlaps the Copilot account
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: v9.0.73-6 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team
Action Performed:
Prerequire - Make sure that your Android device uses 3-button navigation bar Steps:
- Navigate to hybrid app in android mobile (Account A) and https://staging.new.expensify.com in other device(Account B)
- Account B add a copilot to the account (account A)
- In app - Log in to Account A.
- Go to settings and click the account switcher
Expected Result:
Android 3-button navigation bar doesn't overlap the copilot account
Actual Result:
Android 3-button navigation bar overlaps the copilot account
Workaround:
Unknown
Platforms:
- [x] Android: Standalone
- [x] Android: HybridApp
- [ ] Android: mWeb Chrome
- [ ] iOS: Standalone
- [ ] iOS: HybridApp
- [ ] iOS: mWeb Safari
- [ ] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
https://github.com/user-attachments/assets/ac4a9ca3-cbe9-4d1c-9173-e91830a92ad7
Triggered auto assignment to @OfstadC (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.
Edited by proposal-police: This proposal was edited at 2024-12-10 12:05:45 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Android - 3-button navigation bar overlaps the Copilot account
What is the root cause of that problem?
A case was missed during android edge-to-edge implementation
What changes do you think we should make in order to solve the problem?
Change innerContainerStyle and add a condition to retain scrollContainerStyle only on web.
shouldUseModalPaddingStyle={false}
innerContainerStyle={{paddingBottom: unmodifiedPaddings.bottom}}
scrollContainerStyle={!shouldUseNarrowLayout && styles.pb4}
https://github.com/Expensify/App/blob/9975b27ac58d2dd78a521064fd993ed5de097772/src/components/AccountSwitcher.tsx#L203-L204
Like it is implemented here
https://github.com/Expensify/App/blob/9975b27ac58d2dd78a521064fd993ed5de097772/src/pages/Search/SearchTypeMenuNarrow.tsx#L215-L216
result
Search menu and FAB menu for comparison
| Result | Search Menu | FAB menu |
|---|---|---|
Note: unmodifiedPaddings.bottom dynamically computes the padding value based on the host device.
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
NA
What alternative solutions did you explore? (Optional)
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
Edited by proposal-police: This proposal was edited at 2024-12-11 04:44:20 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
- Android 3-button navigation bar overlaps the copilot account
What is the root cause of that problem?
-
This style: https://github.com/Expensify/App/blob/9975b27ac58d2dd78a521064fd993ed5de097772/src/components/AccountSwitcher.tsx#L203-L204 is added via commit on purpose. When we added it, the edge-to-edge mode is not applied.
-
Now, with the edge-to-edge mode, we need to add an additional padding bottom space to the bottom of the modal to make sure the 3-button navigation bar will not overlap the Copilot account modal. But, because we are using
innerContainerStyle={styles.pb0}, themodalContainerStyle.paddingBottomin:
https://github.com/Expensify/App/blob/f8f12c2dcb6d3b62663fc4edea50cd543e44e74d/src/components/Modal/BaseModal.tsx#L157
is 0. As a result, the final paddingBottom value in:
https://github.com/Expensify/App/blob/f8f12c2dcb6d3b62663fc4edea50cd543e44e74d/src/components/Modal/BaseModal.tsx#L276
is 0.
- So, the Copilot modal is not applied an additional paddingBottom style to make sure it is not overlaped by the 3-button-navigation.
What changes do you think we should make in order to solve the problem?
- In:
https://github.com/Expensify/App/blob/9975b27ac58d2dd78a521064fd993ed5de097772/src/components/AccountSwitcher.tsx#L203
Instead of using paddingBottom: 0, we can use: paddingBottom: paddingBottom with the paddingBottom is {paddingBottom} = useStyledSafeAreaInsets()
- This fix will make sure the app is not broken in macos chrome platform.
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional)
Will test this this afternoon
I didn't get a chance to test this yet - will tackle on Tuesday when i'm back in office
@OfstadC Whoops! This issue is 2 days overdue. Let's get this updated quick!
Not able to reproduce
I tried on 4 different devices with the 3 button navigation and all copilot accounts were visible to select
I was able to repro this with 6 copilots and pixel 6 emulator
Reproducible in latest build v9.0.76-12
Job added to Upwork: https://www.upwork.com/jobs/~021869485429092398114
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani (External)
~~@ChavdaSachin 's Proposal looks good and worked well while testing; let's go with it.~~
@daledah's Proposal looks good, previously selected proposal has an issue with scrollView's bottom padding style
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@jayeshmangwani The selected solution will break the UI in web, which was added via commit on purpose. Could you help check again?
| Before | After |
|---|---|
@daledah I couldn’t see much of a difference locally. I applied the suggested changes and reviewed it again, but I’m unsure what major differences to look for. Could you add a comparison video for clarity?
I couldn’t see much of a difference locally. I applied the suggested changes and reviewed it again, but I’m unsure what major differences to look for
In the latest main branch, the ScrollView includes a bottom padding style (we move the padding bottom to the scroll view in past PR), but this style is missing after applying the selected solution. You can see the different in my images above.
You can see the different in my images above.
I can’t see the difference locally; could you add a full video instead of just a cropped screenshot?
@jayeshmangwani Here is video:
- Before:
https://github.com/user-attachments/assets/4305df77-a962-42b3-87d1-92d8df7360f6
- After:
https://github.com/user-attachments/assets/5d0c6c51-5788-440c-841b-ce5982892ba1
@daledah Are you sure you’ve applied these changes? I’ve tried a few times, and it’s working fine for me.
@ChavdaSachin, could you please confirm if your proposal might break the ScrollView padding based on this video?
Are you sure you’ve applied these changes
Sure, I applied these changes. Can you post the video, pls?
https://github.com/user-attachments/assets/2367aa52-0c5c-4316-accd-81a463eedffb
Updated Proposal @jayeshmangwani yes, my old proposal breaks the style on web, But I have updated proposal. thanks @daledah.
Change innerContainerStyle and remove scrollContainerStyle
@jayeshmangwani The old version of that proposal suggested that we should remove scrollContainerStyle. I saw in your video scrollContainerStyle is still existed.
I saw in your video scrollContainerStyle is still existed.
I’ve already added the code changes in my previous comment, @daledah, for confirmation. And you mentioned that you have the same code changes! 😄 and you actually confirmed that you applied same changes
@youssef-lr Based on this comment, it seems the selected proposal had an issue that I overlooked. We can go ahead with @daledah 's proposal instead.
@jayeshmangwani Sorry, my bad. I just focus on your added codes without checking the removed codes 🙏
No issues—my bad! Sorry, I just misinterpreted the previously selected proposal.
@jayeshmangwani the other proposal breaks the style on mobile devices, it adds extra unwanted padding. See the video, and additionally you could compare padding with other similar popups ie. FAB and SearchMenu.
https://github.com/user-attachments/assets/7f71eec7-03e6-449d-8e29-6fb53ae77896
@ChavdaSachin The extra padding bottom comes from commit, it is expected.