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.46k stars 2.81k forks source link

[Ideal Nav] [$1000] [Tracking] Update navigation to support wide layouts on native #35854

Open roryabraham opened 8 months ago

roryabraham commented 8 months ago

Slack context: https://expensify.slack.com/archives/C07J32337/p1707147391183469

Problem

We have an iPad app, and it doesn't work as expected, because we have code that assumes that any code in index.native.js must be representing a narrow layout. Using platform as a proxy for:

is a bad practice and a source for technical debt. It also violates one of the driving philosophies of this app - cross platform 99.99%.

For the sake of scope management, this issue will focus on removing instances where platform is used as a proxy for device layout, specifically these known issues, which will soon be fixed via a temporary workaround.

Solution

Remove code in index.native.js files that assumes it will be running on a narrow layout. Let's get wide layouts working on an iPad Pro (in portrait mode). When this was investigated before, one of the key difficulties found was that we rely on position: fixed in wide layouts on web, but that style is not currently supported in React Native.

This does not mean we will allow the native apps to rotate into a landscape mode.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01290d6331a572c985
  • Upwork Job ID: 1754814923483574272
  • Last Price Increase: 2024-08-08
adamgrzybowski commented 4 months ago

Hi guys! This issue is not easy, so I want to share some of my finding. Maybe this will help in some way!

I see a lot of progress with the above approach, but I was wondering if you had considered using two StackViews? Especially if communication between two NavigationContainers proves difficult to implement.

I tried putting two StackViews in the render function for a custom StackNavigator.

One thing that seems to be beneficial is that we only change the way the screens are displayed. I think this may require less modification to the logic since we are only using one NavigationContainer.

The disadvantage is that on a small screen we would have to create the transition animation from the sidebar to the central panel screens ourselves. This shouldn't be too difficult though.

Alternatively, we could consider implementing our SplitStackView based on the original StackView, but with the ability to display screens side by side

I am including very early POC of this idea below:

cc: @rezkiy37 @mananjadhav

image image
melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @greg-schroeder (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

sakluger commented 4 months ago

@greg-schroeder assigning you to help watch this issue while I'm OOO through May 31. Please handle payment if needed. I'll take back over if it's still open when I return.

mananjadhav commented 4 months ago

Thanks @adamgrzybowski I was exploring SplitView instead of rewriting the NavigationContainer, but I didn't make any progress. I am not sure if we are supporting orientation at the moment, but do you think it could cause any issues there?

Also do you think handling the back actions/navigations would be challenging if we have nested navigators such as Tab navigator in one the StackViews and Stack navigator in the other one?

This definitely sounds like a better plan than going for independent NavigationContainers. Lastly do you think if this will add any overhead in terms of performance/memory?

adamgrzybowski commented 4 months ago

@mananjadhav

I am not sure if we are supporting orientation at the moment, but do you think it could cause any issues there?

We don't support it yet but I guess it should be supported if we manage to support wide layouts on native.

Switching between narrow and wide is potentially problematic, but we have it on the web, and it works in most cases. I think it is important to handle switching the same way on the web and native.

Also do you think handling the back actions/navigations would be challenging if we have nested navigators such as Tab navigator in one the StackViews and Stack navigator in the other one?

This definitely sounds like a better plan than going for independent NavigationContainers. Lastly do you think if this will add any overhead in terms of performance/memory?

So currently we achieve wide layout on the web with styles that move the first StackView card a little left and therefore making it visible. The first card is the BottomTabNavigator and includes every left pane screen.

If we decide to use multiple StackView* components with similar strategy which is to move first rendered route a little left, then I don't think it will make any significant difference in terms of navigation logic and performance.

Also, there is one thing worth keeping in mind. I saw @JmillsExpensify mentioned about upcoming three pane view. It would be great to get more details about the new requirements. There is a possibility that if we want to have three pane view then the approach with two navigation containers will show some advantages that we are not aware now.

*We can also think about creating our own SplitView based on the react-navigation StackView but the idea is similar.

cc: @WojtekBoman @Kicu for visibility

mountiny commented 4 months ago

We don't support it yet but I guess it should be supported if we manage to support wide layouts on native.

Yeah I would think we should build this with landscape orientation in mind

There is a possibility that if we want to have three pane view then the approach with two navigation containers will show some advantages that we are not aware no

Yep that is a good point. I think that we would like to explore this option (its similar to how slack has LHN, main report and a thread in RHP), although I am not sure if right now is the best time yet.

mountiny commented 4 months ago

We are most likely not going to handle the three pane view until after August

mananjadhav commented 4 months ago

Good discussion, thanks for clarifying the details. Working on the implementation. Should have something in a day or two.

greg-schroeder commented 4 months ago

Thanks @mananjadhav!

mountiny commented 4 months ago

@mananjadhav how is it looking

mananjadhav commented 4 months ago

I tried working on both the approaches and I had some challenges. But @adamgrzybowski's approach is more promising so that's what I'll work on now. I'll have more updates by the weekend.

greg-schroeder commented 4 months ago

Appreciate the update!

greg-schroeder commented 3 months ago

Any updates here @mananjadhav? Thanks!

mananjadhav commented 3 months ago

Apologies for the delay, I was catching up on all issues after past few sick days.

I am working on building the SplitStackView within the existing component. I am also trying to get rid of the isSmallScreenWidth check on the responsive navigator.

https://github.com/Expensify/App/assets/3069065/96d378c1-ab48-4563-8095-e372d22b7f8a

I'll push more updates once I make progress on fixing the issue for all platforms. As you can see it's in a funny shape at the moment.

Outside of this issue, related to navigation. One of the issues I am working on also requires a way to access the search route from the RHP (thread). I am checking if we could get rid of this as well.

sakluger commented 3 months ago

Hey @mananjadhav, what's the latest update on this issue?

mananjadhav commented 3 months ago

I haven't made good progress on this on the weekend. I'll focus on this PR this week.

mananjadhav commented 3 months ago

I'll have the a draft PR to work on Web and iPad by the weekend. Mobile is still a problem but I'll open up a draft PR to confirm the approach and get feedback before getting to fix Mobile app.

mountiny commented 2 months ago

@mananjadhav how is the progress looking?

sakluger commented 2 months ago

@mananjadhav any updates?

mananjadhav commented 2 months ago

I couldn't get this working for the native apps. I am going to put another effort this weekend, else will ask for help/reassign. Apologies for the delay here.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sakluger commented 2 months ago

Hey @mananjadhav, did you have any luck with this one?

BrtqKr commented 1 month ago

Hey, I'm a developer from SWM I'd like to take over this ticket

adamgrzybowski commented 1 month ago

Hey! @BrtqKr and I had a meeting where I gave my insights and we did a little brainstorming.

He can start the research, but our current idea for a solution depends on the changes I want to make, described here.

These changes are still in progress and I'll be OOO for two weeks soon, so I want to let you know that work on this issue will probably start after I get back.

Besides this issue is a tough one so I would like to be around 😄 for @BrtqKr

sakluger commented 1 month ago

Commenting for melvin since we just got an update yesterday.

mountiny commented 1 month ago

Makes sense, though could @BrtqKr already be making some progress on this based on the POC branch you have?

BrtqKr commented 1 month ago

I'm trying to wire this up with the POC, so I might be able to figure this out in advance, but I'll keep you updated on that during this week.

sakluger commented 1 month ago

Hey @BrtqKr any updates?

BrtqKr commented 1 month ago

@sakluger, I had to prioritize workspace rules PRs first. I'm done tho, so I'm working on that right now

BrtqKr commented 3 weeks ago

Had some time to experiment with this, this week. With some help from adam we've applied this layout for the tablets using the poc with split stack. I'm trying to research the animations/styling atm, since it's a bit problematic.

We've got one question about the layout though - do we always want to have a split nav in the portrait layout for the tablets, or maybe just for a landscape? It seemed a bit too narrow in some cases, but there are apps that are approaching it in this manner.

mountiny commented 3 weeks ago

We've got one question about the layout though - do we always want to have a split nav in the portrait layout for the tablets, or maybe just for a landscape? It seemed a bit too narrow in some cases, but there are apps that are approaching it in this manner.

I don't think this should ever depend on the device and orientation technically; the layout should always be dictated by the viewport size, so if the tablet portrait mode is too narrow, then we would just use narrow mode. Basically the viewport width should be the source of truth for what layout to render

@shawnborton do you agree?

shawnborton commented 3 weeks ago

Totally agree with that Vit, cc @Expensify/design for the gut check too.

dannymcclain commented 3 weeks ago

100% agree with Vit and Shawn.

BrtqKr commented 3 weeks ago

Hey, since most of the things I'm doing atm are just adjustments to the core part, I wanted to give a quick update on how it looks right now. Currently, the navigation is being rendered correctly in the split variant. Regarding the previous question, I mostly meant the tablet breakpoint, but overall it should be quite easy to change afterward. The video below presents a split variant for width >= medium size breakpoint. As you can see, the styling is mostly done, but I'm still trying to properly handle overlays and some more unique places, such as search. I've also cleaned up the breakpoint conditions in multiple places.

The next steps I'm going to take would be:

  1. Finish overlays
  2. Experiment with an alternate split stack structure to determine proper state persistence while resizing
  3. Test animations
  4. Clean up the breakpoints
  5. Optimizations if necessary

I'll try to provide you with a full version sometime in the next week

https://github.com/user-attachments/assets/19d2166d-9720-4f5b-9a96-40a395679f1b

mountiny commented 2 weeks ago

Thanks @BrtqKr! Great to see the progress, please try to post some update every day you work on this, even if there is not much to show visually.

To confirm, are your changes based on the updates from @adamgrzybowski and @WojtekBoman to refactor some of the navigation logic for bottom tabs history persistence? I think you said yes before but in general i assume we want to base this pr on top of it, so we do not have to do more work later

BrtqKr commented 2 weeks ago

Sure, sorry for that, I've been switching between nav and rules tasks recently, so it wasn't consistent enough to give anything detailed.

Regarding the update for today - I've been working on the custom style interpolator for the modal screens. Everything is related to the animations while transitioning between the report and rhp. I'm making some progress, but I'm somewhere in the middle - there some bugs, which are probably related to the nested stacks, but I'm trying to confirm this

adamgrzybowski commented 2 weeks ago

BTW just to confirm, yes it's based on our changes for the bottom tabs.

BrtqKr commented 2 weeks ago

I've finished the Overlays part, moving to the alternate split stack structure part. Also, we've been testing rendering two screens in the navtive-stack and it will probably require some adjustments, but @adamgrzybowski will be taking care of that.

BrtqKr commented 2 weeks ago

@mountiny , We've been verifying with @adamgrzybowski and @WoLewicki how those changes could be applied if the PR including native-stack gets merged and we've noticed a couple of things. Firstly, the split stack we've been mostly worried about seems to be working as expected, but we had to slightly adjust the split stack structure. Unfortunately, there are some issues when trying to include slide-in animations for the modals, which would be slightly weird if the original intent was to have a native experience.

There are some ways to approach this. Here's a list of options we've considered:

  1. We could create a modal that would appear without any transition, then just apply the slide for the content using reanimated. This is basically writing our own stack with effects, which is quite ugly and might be annoying to maintain afterward.
  2. We could just assume that the modal appears with fade, or something different, but as I stated before, this isn't the native experience we could expect from the modal.
  3. There's an option to keep it in the state from the native stack PR, that is keep is as a full-screen modal. But since we'd like to include horizontal layout in the conditions, this is probably not an option.
  4. We might want to keep some of the modals as a standard stacks, just for the sake of visual flexibility.

We'd probably suggest option 4, but there might still be some other problems related to the native stack we might need to consider later.

mountiny commented 1 week ago

Sounds good, lets just see how the options will play out with the bottom tabs + native stack in

BrtqKr commented 1 week ago

Had to take care of the extension ticket today. I'll be testing option 4 some time tomorrow or on Wednesday.

BrtqKr commented 1 week ago

Still focused on the share extension.

BrtqKr commented 1 week ago

Had some time to start applying the solution, but it's still work in progress, not seeing any issues with this approach so far though

BrtqKr commented 2 days ago

@mountiny, overall the rendering of the native screens seems to be working in combination with the standard ones, but regarding the modals - we've verified that there might be a risk with rendering increasingly larger parts of the stacks without applying native stack, so before we go further we wanted to confirm if this approach is acceptable.

  1. We'll need to verify if the RHP as a standard stack with Report split stack written using native stack screens is working as expected. The main issue was the usage of the cards in the native stack and lack of possibilities for customizing modals. As far as we've checked this is unlikely this approach would work, but we'll try to push it slightly further
  2. If there is no chance the first point would work, this might be necessary to rewrite the whole root stack to the previous state and avoid using native screens in there entirely, just to get the customizable modal handling - this is the main reason I prefer to ask before we dig any deeper, because this seems like a very impactful change especially while having native stack PR being worked on in parallel

We still believe option 4, that is avoiding native stack usage for some of the stacks would be the correct way of handling this scenario, but we wanted to make sure you're aware of where this might lead us.

mountiny commented 2 days ago

@BrtqKr, thanks for the update; we are hoping to get the native stack PRs merged soon, so we will be able to see soon. I would say you can try to pull the native stack changes and work based off that, but if you think that its better to wait for those changes to land on main and only then dig deeper, I can put this on hold too