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.35k stars 2.77k forks source link

[$500] Login - A black underlay appears when pulling down the welcome modal #35547

Closed lanitochka17 closed 7 months ago

lanitochka17 commented 7 months 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: 1.4.35-0 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: Applause - Internal Team Slack conversation:

Action Performed:

  1. Log in with a new account
  2. Pull down the welcome modal

Expected Result:

The background will reveal LHN

Actual Result:

A black underlay appears when pulling down the welcome modal.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/273bc70a-0820-47be-aa30-ec0787634b02

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bbb767262af41b41
  • Upwork Job ID: 1753042822704033792
  • Last Price Increase: 2024-02-01
melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

bernhardoj commented 7 months ago

Proposal

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

Dragging down the welcome modal shows a black background.

What is the root cause of that problem?

The modal is wrapped with a ScrollView https://github.com/Expensify/App/blob/b6363778127003ce5ecb7375c30bb5d05d77b7d9/src/components/PurposeForUsingExpensifyModal.tsx#L147

and on iOS, you can "bounce" the scroll by dragging it at the edge. The bg color is black because we didn't set any bg color to it.

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

If we expect the bg color to be purple color, then we need to move the bg color from the child View to the scroll view. https://github.com/Expensify/App/blob/b6363778127003ce5ecb7375c30bb5d05d77b7d9/src/components/PurposeForUsingExpensifyModal.tsx#L147-L148

Then, we need to wrap the menu items view with a new View that will has a bg app color. https://github.com/Expensify/App/blob/b6363778127003ce5ecb7375c30bb5d05d77b7d9/src/components/PurposeForUsingExpensifyModal.tsx#L164-L176

https://github.com/Expensify/App/assets/50919443/992fdbc8-2d01-4563-a452-bb4f2a60e710

What alternative solutions did you explore? (Optional)

Disable the bounce

Or

We can borrow the solution from HeaderPageLayout by having an over scroll spacer. https://github.com/Expensify/App/blob/9ab4fe92dcbac8252998febf6fa24a080a196a70/src/components/HeaderPageLayout.tsx#L84-L91

https://github.com/Expensify/App/assets/50919443/ce4c78b7-22e5-4f07-8014-51df7659bb23

(the close button is put inside the scroll view, so it gets scrolled too)

shahinyan11 commented 7 months ago

Proposal

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

Login - A black underlay appears when pulling down the welcome modal

What is the root cause of that problem?

Because the modal container has specified background color here

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

  1. Replace this line with the code below
    innerContainerStyle={{...styles.pt0, backgroundColor: 'transparent'}}
  2. Replace this line with below code
    <ScrollView contentContainerStyle={{backgroundColor: theme.highlightBG}}>

    What alternative solutions did you explore? (Optional)

bernhardoj commented 7 months ago

Proposal updated. Added another alt solution

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

bfitzexpensify commented 7 months ago

How do these proposals look @cubuspl42?

cubuspl42 commented 7 months ago

@bernhardoj @shahinyan11

Expected Result:

The background will reveal LHN

How do your proposals relate to the stated expected behavior?

bernhardoj commented 7 months ago

@cubuspl42 Oh my bad, I didn't read the expected result. I think the QA confuses the expected result. Our bottom docked modal never has the ability to swipe down to close. It's disabled. https://github.com/Expensify/App/blob/20e144d5d10f0ec790948fb3db6cad51262f4ef2/src/styles/utils/generators/ModalStyleUtils.ts#L185

It gives a sense that the modal can be closed with a swipe, but it's because the modal is scrollable.

shahinyan11 commented 7 months ago

How do your proposals relate to the stated expected behavior?

@cubuspl42 I don't see anything about closing the modal window in the expected result. As far as I understand, when moving the modal down, the LHN should be visible in the background.

cubuspl42 commented 7 months ago

@shahinyan11 Oh, so you actually had that the stated expected result in mind. Would you add a video to your proposal, showing the achieved effect?

shahinyan11 commented 7 months ago

@cubuspl42

https://github.com/Expensify/App/assets/46921547/2cf753c9-8009-49af-8248-e82f020a4828

cubuspl42 commented 7 months ago

@shahinyan11 Looks good! I think we can go forward with this solution.

I approve the proposal by @shahinyan11

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 7 months ago

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

bernhardoj commented 7 months ago

@cubuspl42 do you have any thoughts on my previous comment? I don't think we really want to make it transparent.

https://github.com/Expensify/App/assets/50919443/21c1e2c4-52da-403c-9176-6744b03fc52a

We can additionally ask @stitesExpensify the expected behavior as he is the one who implements this page/modal.

cubuspl42 commented 7 months ago

do you have any thoughts on my previous comment?

Not really. I question the expected behavior when I find it unintuitive; it wasn't the case here. It actually feels natural to see the LHN behind this modal.

MonilBhavsar commented 7 months ago

Proposal looks good but would like to get extra set of eyes from @Expensify/design team. cc @stitesExpensify

bernhardoj commented 7 months ago

I don't think it's natural for this case, right?

image

Also, making it transparent makes it even more sense that the modal is swipeable and can be closed, but in reality, is not possible which feels broken as a user.

IMO, it feels more natural if we allow the swipe on the modal.

shawnborton commented 7 months ago

Yeah, I agree that this modal view shouldn't be able to be scrolled like that. @stitesExpensify can you take a look and weigh in here as well since you implemented this?

shawnborton commented 7 months ago

We might consider just making this a full screen modal on mobile - I actually could have sworn that was the case? That would prevent a lot of this weirdness.

shahinyan11 commented 7 months ago

I just suggested a solution according to the expected result. If there should be another behavior we can clarify it and provide another solution

stitesExpensify commented 7 months ago

@shawnborton what do you mean by a full screen modal? Do you mean like the settings pages?

shawnborton commented 7 months ago

Correct - I would think it would occupy the entire height/width of the mobile screen, and not appear as a bottom-docked modal.

stitesExpensify commented 7 months ago

I think that is a significant refactor (a contributor can feel free to prove me wrong on that though). If I am correct, then I don't think it is worth changing given that we are going to replace it with Stage 1 Guided Setup soon. If we were not going to replace it, do you have a second favorite solution?

shawnborton commented 7 months ago

Ah fair, if we're going to replace this, let's do nothing.

If not, we should just make this a full screen page like other RHP modals.

stitesExpensify commented 7 months ago

Cool, it looks like the detailed design already has ~1/2 of its reviews, so I'm going to close this out.