Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

[$250] When using the account switcher on iOS there is no safe area #48292

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks ago

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.26-1 Reproducible in staging?: Y Reproducible in production?: Y 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: @AndrewGable Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1724946621382309

Action Performed:

1.Launch the app and login

  1. Tap settings > Account switcher arrows

Expected Result:

No display issues and modal view is scrollable

Actual Result

There is no safe area when using the account switcher

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

unnamed

IMG_1397

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021831817133463086139
  • Upwork Job ID: 1831817133463086139
  • Last Price Increase: 2024-09-05
  • Automatic offers:
    • nkdengineer | Contributor | 103922536
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @s77rt
melvin-bot[bot] commented 2 weeks ago

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

dangrous commented 2 weeks ago

cc @rushatgabhane on this one

RachCHopkins commented 2 weeks ago

Launch the app

Which app? Hybrid app?

Tap settings > Account switcher arrows

I don't know what this means.

Can we get some more detailed instructions for testing/reproducing, please?

RachCHopkins commented 2 weeks ago

Ok, looks like it's:

Launch the New Expensify app. Go to Settings > tap username at the top to switch accounts

I can confirm I can't scroll in the list. I'm not sure what "safe area" means, but if I try to select my own account, the app crashes.

RachCHopkins commented 2 weeks ago

Waiting to see what @rushatgabhane has to add here! However this is supposed to work, it's definitely not working like it should.

dangrous commented 2 weeks ago

safe area means the area where default phone interface stuff should be (like in the screenshot, it's overlapping with the wifi symbols, etc.)\

I think we can go ahead and assign @rushatgabhane here for a solution! I can also take this on from the engineering side

melvin-bot[bot] commented 1 week ago

@dangrous, @rushatgabhane, @RachCHopkins Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

dangrous commented 1 week ago

We might want to make this external to clear @rushatgabhane's plate since a contributor should be able to propose

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

Current assignee @rushatgabhane is eligible for the External assigner, not assigning anyone new.

RachCHopkins commented 1 week ago

@dangrous do we need to remove @rushatgabhane?

ItxAltaf commented 1 week ago

You can use SafeAreaView property if you are working with react native

melvin-bot[bot] commented 1 week ago

πŸ“£ @ItxAltaf! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
ItxAltaf commented 1 week ago

Contributor details Your Expensify account email: itxaltaf789@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01ff545e274abdf999?mp_source=share

melvin-bot[bot] commented 1 week ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

kishan-dhankecha commented 1 week ago

You can add a SafeArea widget as the scaffold body. So that AppBar and such things won't affect with the padding, it is also safe for content.

melvin-bot[bot] commented 1 week ago

πŸ“£ @kishan-dhankecha! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
kishan-dhankecha commented 1 week ago

Contributor details Your Expensify account email: dhankechakishan@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/kishandhankecha

melvin-bot[bot] commented 1 week ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

nkdengineer commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-09-09 08:10:33 UTC.

Proposal

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

There is no safe area when using the account switcher and the list is not scrollable

What is the root cause of that problem?

We don't apply SafeAreaConsumer for account switcher popover and we don't wrap this component with a ScrollView then it's not scrollable

https://github.com/Expensify/App/blob/da8370fd434594a2ade67a81035a6caec0d0118c/src/components/AccountSwitcher.tsx#L169

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

We can use the SelectionList to fix both the safe area and the scroll bug.

  1. We should update createBaseMenuItem that will return the item with the type ListItem for SelectionList. For the badge, we can return rightElement in this function that is Badge component with text is additionalProps.badgeText

https://github.com/Expensify/App/blob/c6d8b635623e00f614d439ff13a2367a4a8bf199/src/components/AccountSwitcher.tsx#L48

  1. For this text we can pass headerMessage and headerMessageStyle to SelectionList

https://github.com/Expensify/App/blob/c6d8b635623e00f614d439ff13a2367a4a8bf199/src/components/AccountSwitcher.tsx#L179

  1. To set the max height of the modal we should pass maxHeight style into innerContainerStyle prop of the Popover and pass containerStyle prop as styles.flex0 into SelectionList to prevent the SelectionList take the max height of the modal.

https://github.com/Expensify/App/blob/c6d8b635623e00f614d439ff13a2367a4a8bf199/src/components/AccountSwitcher.tsx#L169

We can see the implementation details here

What alternative solutions did you explore? (Optional)

We can use PopoverMenu for this case

  1. We should update createBaseMenuItem to return the data with the type PopoverMenuItem

  2. When use PopoverMenu, we should pass anchorPosition with horizontal: 12 and vertical: 80 to match with the old position on web. Also, need to pass anchorAlignment with vertical as top. For this text we can pass headerText prop

https://github.com/Expensify/App/blob/c6d8b635623e00f614d439ff13a2367a4a8bf199/src/components/AccountSwitcher.tsx#L179

  1. We should wrap the list item here into a ScrollView and then the popover menu can be scrolled

https://github.com/Expensify/App/blob/286b9451b1d2834c6624993fe176a57925763ff9/src/components/PopoverMenu.tsx#L253

We also need to add an extra prop to like containerStyles for PopoverMenu then in AccountSwitcher we can pass the maxHeight style as windowHeight / 2 for this view then we can dismiss the popover by clicking outside and doesn't face the safe area issue

https://github.com/Expensify/App/blob/286b9451b1d2834c6624993fe176a57925763ff9/src/components/PopoverMenu.tsx#L250

Result

https://github.com/user-attachments/assets/c6da2470-8715-4c18-9aff-7ccfcb067c7a

dangrous commented 1 week ago

yeah let's grab another C+ so @rushatgabhane can focus on existing issues. I'll remove and then try reapplying the External label.

melvin-bot[bot] commented 1 week ago

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

s77rt commented 1 week ago

@nkdengineer Thanks for the proposal. Your RCA is correct. Regarding the solution, can we use SelectionList instead? It should cover both SafeAreaConsumer and ScrollView.

nkdengineer commented 1 week ago

@s77rt I just tested, on native, it only displays the header. on Web the scroll issue isn't fixed.

s77rt commented 1 week ago

@nkdengineer Can you elaborate? How come only the header is visible? You should be able to make it work. If not, can you explain the problem?

nkdengineer commented 1 week ago

I tested with a View that wrap the SelectionList, if I fix the height of View component it works on both web and native. If not the bugs I mentioned above happens.

s77rt commented 1 week ago

@Expensify/design How is the account switcher list supposed to look when the list contains too many accounts?

Currently it's broken, no safearea consideration, the list is not scrollable and the popover cannot be dismissed

Screenshot 2024-09-07 at 6 09 55β€―AM

After fixing the safearea and the scroll issues, it will look like this but it's still not dismissible

Screenshot 2024-09-07 at 6 10 08β€―AM

Can we have a design view for this case? And is the push to page an option here? (like we done with the workspace switcher)

rushatgabhane commented 1 week ago

@s77rt i think there would be a max height (50% of the screen) so you can still tap outside to close the modal

shawnborton commented 5 days ago

I think push to page is a bit heavy for this solution - I think a majority of the use cases won't have so many copilots that it fills the screen.

I agree with the comment about making the modal have a smaller max-height though. Perhaps we cap it at something like 75% or something so there is still amply area to tap to close outside of it?

s77rt commented 5 days ago

Sounds good

s77rt commented 5 days ago

@nkdengineer Can you update your proposal based on the above? (and with SelectionList)

rushatgabhane commented 5 days ago

If we're using SelectionList then keyboard should navigation should work too, right?

It'll also fix https://github.com/Expensify/App/issues/48245 then

nkdengineer commented 5 days ago

@s77rt Update my proposal

s77rt commented 5 days ago

@rushatgabhane It will not fix the arrow navigation issue because the SelectionList arrow activation logic is based on screen focus. PopoverMenu on the other hand works because the activation logic is based on the visibility of the modal.

PopoverMenu may be the more appropriate option here but I think it lacks scroll support. (It also lacks the safe area support but that shouldn't be a concern anymore)

cc @nkdengineer Can you update your proposal to support keyboard navigation as well

nkdengineer commented 5 days ago

Let me check again.

nkdengineer commented 3 days ago

@s77rt I updated proposal with alternative solution for PopoverMenu.

s77rt commented 3 days ago

@nkdengineer Thanks for the update. The proposal overall looks good to me.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed Link to proposal

melvin-bot[bot] commented 3 days ago

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

dangrous commented 2 days ago

works for me!

melvin-bot[bot] commented 2 days ago

πŸ“£ @nkdengineer πŸŽ‰ 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 πŸ“–

nkdengineer commented 2 days ago

@s77rt The PR is ready https://github.com/Expensify/App/pull/49051.