[HOLD] Replace modal screens with modals from `@react-navigation`

[chrispader]

This issue is just to keep track of this proposal and eventually create a P/S in Slack

Problem

As discussed in this issue and this Slack thread we should replace modal screens (navigation screens/routes, that only render a modal) and modals that stay open when the containing screen gets unmounted.

The current modals use react-native-modal which uses RN internal Modal under the hood. Using these modals with @react-navigation/native-stack poses issues with animations and navigation, as these modals cannot animate in/out, due to the screen mounting/unmounting at the same time. Additionally, the animations will not look the same as the rest of the screen animations, which doesn't look very nice.

Solution

Replace the following types of modals with modal (screens) from @react-navigation

• screens/routes that only render a modal
• modals on screens, that are navigated back from while the modal is still open or animating out at the same time

cc @mountiny

[melvin-bot[bot]]

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

[mountiny]

@chrispader we will have to discuss this and make a proposal first

[melvin-bot[bot]]

@chrispader, @mountiny, @Christinadobrzyn Eep! 4 days overdue now. Issues have feelings too...

[Christinadobrzyn]

Hey @mountiny or @chrispader - should this remain a daily issue?

Are we holding it for this PR - https://github.com/Expensify/App/issues/53362

[mountiny]

No, we are holding this for a proposal/ discussion around it

[mountiny]

Made chris the owner of the issue

[Christinadobrzyn]

Just a heads up that I'm going to be ooo Dec 12 - 13th. Back on Monday 16th. I'm not going to assign this to a BZ teammate but if anything is urgent, please reach out to the team for a volunteer.

[chrispader]

I've just posted a P/S thread for this here: https://expensify.slack.com/archives/C01GTK53T8Q/p1733938583809829

[Christinadobrzyn]

Nice work on the P/S @chrispader - looks like it has lots of votes to move forward. Let us know what you're thinking for next steps!

[melvin-bot[bot]]

@chrispader, @mountiny, @Christinadobrzyn Huh... This is 4 days overdue. Who can take care of this?

[mountiny]

I think w can take this off hold, adding this to quality as low as I think it aims to improve UX.

@chrispader can you please start with a plan on how to tackle this before writing any code. Also would be great to get agreement from @adamgrzybowski @WoLewicki on the next steps and maybe even work together with them as this is change close to screens and navigation

[WoLewicki]

I'd also add @BartoszGrajdek who is reworking some of the modals to work with reanimated.

[melvin-bot[bot]]

@chrispader, @mountiny, @Christinadobrzyn 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[mountiny]

@chrispader Do you have any ETA for moving this issue ahead? Do you need to hand over some of the other tasks to someone else in Margelo maybe?

[melvin-bot[bot]]

@chrispader, @mountiny, @Christinadobrzyn 10 days overdue. I'm getting more depressed than Marvin.

[chrispader]

@chrispader Do you have any ETA for moving this issue ahead? Do you need to hand over some of the other tasks to someone else in Margelo maybe?

I'm going to work on a proposal for this until the end of the week.

In the proposal i will skim through the app and try to find all the screens where it pays off to migrate them to modal screens.

[chrispader]

@Christinadobrzyn @mountiny i added a WIP list of components and screens to migrate to the issue description. I'm actively working on adding all the components and screens, that we should or can migrate.

Anybody involved in current work around modals and animation, feel free to let me know about any further cases that you know of that are problematic or worth migrating.

cc @BartoszGrajdek @WoLewicki @adamgrzybowski

[chrispader]

Approach / Proposal

Triage all screens and components that contain modals into strictly necessary, beneficial and potential but not necessary issues.

Once we've triaged all modals in screens and components into the three priorities/categories (described in the issue description) we can start migrating those components to use modal screens from @react-navigation. The migration will look somewhat like this:

1. Extract the modal (screen) content to a separate React component, if not already done so. Think about all the navigation props that are needed for the modal screen, to behave like before.
2. Create one or many new routes for each case in any flow, where the modal appears. e.g. the AmountPicker modal in the workspace taxes flow as something like /workspaces/wallet/add/amount.
3. Replace any usages of modals by (re-)navigating to the new screen with the appropriate props.
4. Test that functionality behaves the same as before.
5. (Potentially test direct navigation linking to the modal, if it can be reached through a URL directly)

[melvin-bot[bot]]

@chrispader, @mountiny, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[chrispader]

I've now updated the list of modals to the extent, that it includes all modals used in the app (that i could find through "smart" file search in VS Code and using AI Tools) and it's usages across the codebase.

Lots of the files (modal components, other components, pages, etc.) listed under "TODO" appear more than once, but might not need to be handled individually.

My next step would be to skim through the app (both in the web version and on iOS/Android) and check all of these modals and triage them by the described categories.

[parasharrajat]

I would like to be C+ for this migration.

[Christinadobrzyn]

Awesome! Should I add you to this issue @parasharrajat - not sure if there's a different process for C+ on a new project.

[parasharrajat]

Let me ask Matt for the process and check some documentation but I available to take this on.

[parasharrajat]

@Christinadobrzyn looks the process is simple. You can assign me. If this is a project, I will be assigned to all PR/issues.

[Christinadobrzyn]

Awesome thanks @parasharrajat! Sorry I should've asked Matt - thanks for doing that.

[parasharrajat]

@Christinadobrzyn I didn't ask Matt. I looked up the C+ doc which does not say anything against it. If you think we should ask Matt, please do that.

[Christinadobrzyn]

No I think that's fine @parasharrajat! Thanks for checking though!

[chrispader]

I've just finished triaging all the modals in the codebase. (Hopefully i found all 😅)

For each of the modals, you can find a detailed list of all the screens and components that use it. The different modals are prioritized into the three described categories, 🔴 HIGH, 🟡 MEDIUM, and 🟢 LOW.

The result from the triaging process now opens the following questions that need to be answered, before we want to start migrating the modals:

• Do we only want to fix modals that currently cause issues (🔴) or also other "screen-like" modals (🟡), to fix consistent animations, gestures and usual screen navigation behavior?

  (In the later case, we definitely want to split up the work between high and medium priority modals)

• Do we want to work on all of this in one PR that migrates all modals or should we split up the workload into multiple PRs (and multiple assignees)?

• We'll probably have to re-write the BaseModal, Popover and maybe other "base components", to work differently on web vs. native. Do you want me to work on this in a first "major PR", that the other PRs should hold on?

cc @Christinadobrzyn @mountiny

Also, thoughts from @adamgrzybowski @WoLewicki @BartoszGrajdek are welcome 😄

[BartoszGrajdek]

Hey Chris, thanks for mentioning me here! 🙌🏻 Your plan looks solid, and I really don't have much to add. 🪨

Just a quick recap on what I've been up to (you've probably already seen it, since you linked it in your Slack message):

I'm currently wrapping up a PR for some enhancements to modals (here's the issue). Our main goal was to swap out react-native-animatable with reanimated in react-native-modal library, targeting all modals in E/App across both native and mWeb platforms to improve the animation performance.

We decided to roll out these improvements starting with the bottom docked modals, which are currently on your 🟢 LOW priority list. I understand you'll be handling the other modal types. So, I'll hold off on those unless plans change, or you decide that any other modals should stick to the previous solution – just let me know if they do! 👀

I'm planning to raise a PR later this week. I'll tag you there so that you can take a look at it - there are some really small changes in BaseModal, but nothing that should worry you.

Also, you mentioned a bug in the 🔴 HIGH priority -> ConfirmModal. Good news – that issue is resolved with our new setup using layout animations, which allows us to execute a callback immediately after an animation takes place, even if the modal unmounts. I've attached a video below demonstrating the new flow. Please take a look! 😄

ConfirmModal Demo

https://github.com/user-attachments/assets/c0363cb2-9b0f-4784-a8cb-19723e577107

Let me know if you have any questions - I'm happy to help! 😊

[mountiny]

Thank you @chrispader for a great audit.

From my perspective, it seems like 🟢 LOW is actually solved by the work @BartoszGrajdek and his team are doing to the bottom sheets modal. What would be the benefit of migrating this to the react-navigation modal? Feels like we can leave it as is.

Then I agree that Attachment modal seems like the main pain point to look into. It also seems like the confirm modal might be 🟢 after the changes from SWM.

So, I recommend you start with the AttachmentModal, create the alternative using react-navigation, and only apply it in one case first. Then, after testing and deployment, we can create PRs in other places, slowly deprecating the old modal.

For the 🟡 it seems to me that all of those screen-like modals would benefit from being react-navigation models so we could once again migrate them over in small prs to make sure any regressions can be fixed easily.

@adamgrzybowski @WoLewicki would you have any objections with these plans? Any reasons why not to migrate these?