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.78k forks source link

[$250] Onboarding - Changing the size of the window dismisses the onboarding modal #48930

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets 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: v9.0.31-4 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): dave0123seife@gmail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Resize the height of the window to be a very small mobile viewport
  2. Sign in as a new user 3 When the onboarding modal shows resize to normal window size

Expected Result:

Resizing the of window does not dismisses the onboarding modal

Actual Result:

Resizing the of window dismisses the onboarding modal

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/3f521fd2-0dbf-486b-b417-9d2acf48fec9

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834487171313044824
  • Upwork Job ID: 1834487171313044824
  • Last Price Increase: 2024-09-13
  • Automatic offers:
    • jjcoffee | Reviewer | 103951138
    • c3024 | Contributor | 103951139
Issue OwnerCurrent Issue Owner: @jjcoffee
melvin-bot[bot] commented 2 weeks ago

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

IuliiaHerets commented 2 weeks ago

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

c3024 commented 1 week ago

Proposal

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

Logging in on a narrow window and then enlarging it to a non-narrow window dismisses the onboarding modal

What is the root cause of that problem?

When we login in the narrow view, the routes in state only have BottomTabNavigator and OnboardingModalNavigator

Screenshot 2024-09-11 at 10 40 34β€―PM

and when we enlarge it we find the template state from the path https://github.com/Expensify/App/blob/bbe7d3814151d27b2f0ee7bae5e6e17d0e670d1f/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L59 and here REPORT screen is added to the template state https://github.com/Expensify/App/blob/56033cbee88f70308f7580c1d730e190797f9f41/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L240-L242 We extract the central pane from the template state and insert it into state if there is no top most central pane route in it https://github.com/Expensify/App/blob/bbe7d3814151d27b2f0ee7bae5e6e17d0e670d1f/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L74-L88 and we insert it before modalRoutes https://github.com/Expensify/App/blob/bbe7d3814151d27b2f0ee7bae5e6e17d0e670d1f/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L26 but here https://github.com/Expensify/App/blob/bbe7d3814151d27b2f0ee7bae5e6e17d0e670d1f/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L19-L20 only Left and Right Modal Navigators are included in the modal routes and Onboarding Navigator is not so it is moved to an index before the REPORT route.

So, the order of navigators will become BottomTabNavigator > Onboarding Navigator > Report and the Onboarding Modal gets dismissed.

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

Exclude and include the NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR in these two lines respectively. https://github.com/Expensify/App/blob/bbe7d3814151d27b2f0ee7bae5e6e17d0e670d1f/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L19-L20

What alternative solutions did you explore? (Optional)

VictoriaExpensify commented 1 week ago

2024-09-13_16-55-03 (2)

Recreated the issue and I agree that this seem like a problem we should fix

VictoriaExpensify commented 1 week ago

It doesn't look like this is being resolved elsewhere

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

jjcoffee commented 1 week ago

@c3024's proposal LGTM!

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

πŸ“£ @jjcoffee πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

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

c3024 commented 3 days ago

Deployed to production on 18-Sep. Payment due on 25-Sep.

jjcoffee commented 37 minutes ago

Regression Test Proposal

  1. Reduce the browser window to a narrow size.
  2. Login with a new account.
  3. Wait till the onboarding modal opens up.
  4. Enlarge the screen till the central pane (report screen) shows up.
  5. Verify that the onboarding modal is not dismissed.

Do we agree πŸ‘ or πŸ‘Ž

jjcoffee commented 36 minutes ago

@VictoriaExpensify This one's due payment now as it was deployed last week.