App icon indicating copy to clipboard operation
App copied to clipboard

Android - 3-button navigation bar overlaps the Copilot account

Open IuliiaHerets opened this issue 1 year ago • 2 comments

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:

  1. Navigate to hybrid app in android mobile (Account A) and https://staging.new.expensify.com in other device(Account B)
  2. Account B add a copilot to the account (account A)
  3. In app - Log in to Account A.
  4. 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

View all open jobs on GitHub

IuliiaHerets avatar Dec 10 '24 11:12 IuliiaHerets

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.

melvin-bot[bot] avatar Dec 10 '24 11:12 melvin-bot[bot]

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
Screenshot 2024-12-10 at 5 32 54 PM Screenshot 2024-12-10 at 5 27 25 PM Screenshot 2024-12-10 at 5 27 39 PM

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.

ChavdaSachin avatar Dec 10 '24 11:12 ChavdaSachin

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}, the modalContainerStyle.paddingBottom in:

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)

daledah avatar Dec 11 '24 04:12 daledah

Will test this this afternoon

OfstadC avatar Dec 11 '24 16:12 OfstadC

I didn't get a chance to test this yet - will tackle on Tuesday when i'm back in office

OfstadC avatar Dec 13 '24 17:12 OfstadC

@OfstadC Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 17 '24 09:12 melvin-bot[bot]

Not able to reproduce image

I tried on 4 different devices with the 3 button navigation and all copilot accounts were visible to select

OfstadC avatar Dec 17 '24 18:12 OfstadC

I was able to repro this with 6 copilots and pixel 6 emulator

ChavdaSachin avatar Dec 17 '24 18:12 ChavdaSachin

Reproducible in latest build v9.0.76-12

Screenshot_20241218_184948_Expensify

izarutskaya avatar Dec 18 '24 16:12 izarutskaya

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

melvin-bot[bot] avatar Dec 18 '24 20:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 18 '24 20:12 melvin-bot[bot]

~~@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

jayeshmangwani avatar Dec 20 '24 05:12 jayeshmangwani

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Dec 20 '24 05:12 melvin-bot[bot]

@jayeshmangwani The selected solution will break the UI in web, which was added via commit on purpose. Could you help check again?

Before After
Screenshot 2024-12-20 at 13 40 50 Screenshot 2024-12-20 at 13 42 14

daledah avatar Dec 20 '24 06:12 daledah

@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?

jayeshmangwani avatar Dec 20 '24 07:12 jayeshmangwani

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.

daledah avatar Dec 20 '24 07:12 daledah

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 avatar Dec 20 '24 07:12 jayeshmangwani

@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 avatar Dec 20 '24 07:12 daledah

@daledah Are you sure you’ve applied these changes? I’ve tried a few times, and it’s working fine for me.

Screenshot 2024-12-20 at 1 17 04 PM

jayeshmangwani avatar Dec 20 '24 07:12 jayeshmangwani

@ChavdaSachin, could you please confirm if your proposal might break the ScrollView padding based on this video?

jayeshmangwani avatar Dec 20 '24 07:12 jayeshmangwani

Are you sure you’ve applied these changes

Sure, I applied these changes. Can you post the video, pls?

daledah avatar Dec 20 '24 07:12 daledah

https://github.com/user-attachments/assets/2367aa52-0c5c-4316-accd-81a463eedffb

jayeshmangwani avatar Dec 20 '24 08:12 jayeshmangwani

Updated Proposal @jayeshmangwani yes, my old proposal breaks the style on web, But I have updated proposal. thanks @daledah.

ChavdaSachin avatar Dec 20 '24 08:12 ChavdaSachin

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.

daledah avatar Dec 20 '24 08:12 daledah

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

jayeshmangwani avatar Dec 20 '24 08:12 jayeshmangwani

@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 avatar Dec 20 '24 08:12 jayeshmangwani

@jayeshmangwani Sorry, my bad. I just focus on your added codes without checking the removed codes 🙏

daledah avatar Dec 20 '24 08:12 daledah

No issues—my bad! Sorry, I just misinterpreted the previously selected proposal.

jayeshmangwani avatar Dec 20 '24 08:12 jayeshmangwani

@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 avatar Dec 20 '24 09:12 ChavdaSachin

@ChavdaSachin The extra padding bottom comes from commit, it is expected.

daledah avatar Dec 20 '24 09:12 daledah