[Main Tracking] App Navigation Reboot

puneetlath commented 1 year ago

Design doc link - https://docs.google.com/document/d/1cQ87HcDA9RrmBM4ch7PmlNi9N9ZP8abQuPF0gKYrxDE/edit

Problem:

We initially implemented react-navigation in App without a proper design doc as part of the NewDot MVP. While this library came with some awesome benefits - we’ve realized over time that some of our early decisions in how we use the library have impacted performance and usability (i.e. bugs and lots of em). Our current design also deviates from our ideal design and needs to be reworked.

Solution:

Let’s establish a ruleset of navigational actions a user can take and define which gestures or interactions relate to those navigational actions. Then we’ll re-implement React Navigation with those rules in mind. We will kill the drawer navigator favoring a simpler stack navigator approach. And finally, unblock the bugs and make our app navigation competitive and bug free.

Timeline: WhatsApp Quality Level Urgency 🔥

Plan of Action:

[x] #whatsnext link
[x] Send Strategy@ email
[x] Predesign links 1, 2, 3
[x] HL Design Doc link
[x] Detailed Design Doc
[x] Implement New Navigation
[x] Unblock bugs by fixing or closing them

Implementation PRs

Overview

This GH will serve as the tracking issue for improving our React Navigation implementation. A bunch of issues with navigation have been found and are referenced in this project.

List of related issues

Currently fall into two major categories

Drawer Navigation / State

Drawer state is unpredictable
Drawer state transition end can not be listened for - https://github.com/Expensify/App/issues/9253
Home screen / MainDrawerNavigator is not getting route params from Report route
Switching from large to small width screens breaks search navigation (actually the drawer is just stuck closed)
- https://github.com/Expensify/App/issues/9734
- https://github.com/Expensify/App/issues/9342
Navigating to search page using a link while logged out ends up in empty option list after signing in
https://github.com/Expensify/App/issues/12128
https://github.com/Expensify/App/issues/16319
https://github.com/Expensify/App/issues/17147

Browser History

Back navigation / history stack for reports on web is hacked in
mWeb navigating back exits the chat (report route not added to history
mWeb visual bug on Safari related to popstate event
- https://github.com/Expensify/App/issues/7737
Back/Forward shortcut keys do not work
After deleting stuff, navigating back can lead to a 404
- https://github.com/Expensify/App/issues/11524
https://github.com/Expensify/App/issues/11453
https://github.com/Expensify/App/issues/10409
https://github.com/Expensify/App/issues/11092
https://github.com/Expensify/App/issues/9823
https://github.com/Expensify/App/issues/12203
https://github.com/Expensify/App/issues/11515
https://github.com/Expensify/App/issues/13449
https://github.com/Expensify/App/issues/13743
https://github.com/Expensify/App/issues/14973
https://github.com/Expensify/App/issues/16557

Performance

Misc.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @CortneyOfstad (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

parasharrajat commented 1 year ago

Get Control of React-Navigation

:fire: :fire:

melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

puneetlath commented 1 year ago

Suggestion from @roryabraham: move src/libs/Navigation to src/Navigation to highlight its importance to someone coming to the repo for the first time.

puneetlath commented 1 year ago

Another interesting note from @roryabraham: we don't use React Navigation's link component.

melvin-bot[bot] commented 1 year ago

@puneetlath Eep! 4 days overdue now. Issues have feelings too...

puneetlath commented 1 year ago

Reached out to React Navigation library owner, but haven't heard back. Will follow up again this week as well as with perhaps some others.

puneetlath commented 1 year ago

Jason will be the BZ person tag teaming this with me, so assigning to him to the issue as well.

melvin-bot[bot] commented 1 year ago

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

JmillsExpensify commented 1 year ago

@puneetlath I see you're trying to get invited to the Discord room, but is there anything else we can do to speed this part up? It's kind of a bummer that getting in contact with the maintainer is a blocker.

puneetlath commented 1 year ago

If I'm not able to get access by tomorrow then we'll reach out to some of our other vendors to see if they can help.

johncschuster commented 1 year ago

Hey @puneetlath, @marcaaron suggested we added this issue to this larger tracking issue you're managing. Just wanted to flag it for you in case it's not already on your list!

puneetlath commented 1 year ago

Got access to Discord today and reached out. Will see what they say and then go from there.

JmillsExpensify commented 1 year ago

Oh nice work!

JmillsExpensify commented 1 year ago

Any luck with next steps on this one?

puneetlath commented 1 year ago

No luck with the maintainers. Have asked margelo if they can help next.

JmillsExpensify commented 1 year ago

Looks like they are willing to jump in based on the Slack response.

JmillsExpensify commented 1 year ago

I posted about this in Slack today, but I'm also kind of thinking that we still don't have any great leads. It sounds like Margelo can likely help – and today is a holiday in Poland – so perhaps let's wait one more day for extra clarity.

JmillsExpensify commented 1 year ago

That said, maybe we should brainstorm other approaches, like what would those be?

Find new library?
See if anyone internal can take this on?
Check with SWM
Anything else?

marcaaron commented 1 year ago

I have familiarity with react navigation and bandwidth to help here if this is a good place to assist.

Does it feel like we have settled on what a new navigation design would look like based on the Slack convo here?

From where I'm sitting, it looks like we did some initial investigation and should compile that into a Navigation 2.0 doc then once there is broad agreement on how navigation will work we can investigate what changes (if any) react-navigation would need? Maybe it's none or at least fewer than we are experiencing now. Bringing people in to fix things related to the drawer navigator might not make sense if our new navigation design replaces it with a stack nav.

puneetlath commented 1 year ago

I would very much welcome your help here @marcaaron. I have not been doing a good job at all moving this forward and am very conscious of the fact that I'm being a blocker for it.

My plan was to write up a summary of where I think we want to go with Navigation for Margelo in the Slack channel, but I like the idea of writing it in a doc instead. That seems like a better approach.

JmillsExpensify commented 1 year ago

Nice, this sounds really great! Let me know if ya'll need help in any way.

laurenreidexpensify commented 1 year ago

@puneetlath as this is a tracking issue, does it need the Bug Label?

JmillsExpensify commented 1 year ago

It's fixing several bugs in the app, so I think it's appropriate.

marcaaron commented 1 year ago

I think it would be good to have some minimal proof of concepts together so people can actually click around and then share opinions on how things should work. I went back through some of the threads yesterday and a few points seem undecided. Will try to summarize everything by the end of today.

marcaaron commented 1 year ago

Spent time today on a minimal proof of concept today that tests out a basic mobile stack flow where we:

Get rid of the x in right hand modal (gotta hit < no x at all)
< button will either bring you to the "parent" node or the previous screen depending on context
- The "Search" view would be in the navigation stack and a navigation like this would be possible:
  - LHN (push) Search (push) Chat 1 (push) Chat 2 (<) Chat 1 (<) Search (<) LHN
- And also that if we navigate to the Settings/About screen via a Chat screen then we would start get something like this
  - LHN (push) Chat 1 (push) [Settings, About] index 1 (<) [Settings] (<) Chat 1 (<) LHN

I'm working on all of this in a Snack because it's easy for people to download the Expo Go app on the platform of their choice and test this stuff out for real. The UI is just placeholder stuff so you have to use your imagination a bit. Just download the app, scan the QR code, and you should be testing.

https://snack.expo.dev/@marcaaron/6e1035

Practically speaking, it is not too challenging to implement this style of view for native. So, I'm focusing on working out native/mobile web for now and we can test how it feels there. Once it's good we can think about how to get it working for desktop.

Using react navigation to use the stack based navigation hierarchy is not immediately obvious to me and may require a custom navigator so that views in the stack can be repositioned or presented however we like (assuming it's even possible). The current examples I've come across for desktop views all use a drawer navigator in the "permanent" position. But this is a good thing to seek advice on.

If anyone wants to test the web implementation you can clone this branch

https://github.com/marcaaron/navigation-test

And run npm i && npm run web.

It only shows a mobile web style view for now, but you can test the browser back button press vs. < press. In the case where we are navigating from a Chat -> [Settings, About] index 1 a back button press would bring you back to Chat, while a < press will bring you to Settings.

However, if you are on the Settings page another back button press brings you back to the LHN. So the browser navigation is not without bugs even when just using a stack navigator and ditching the drawer navigator 😄

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @puneetlath, @marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick!

marcaaron commented 1 year ago

Plan for this week:

Navigation Test

Get a nicer looking proof of concept together then deploy a mobile web test app + invite everyone to test things with Expo Go so we can see how the behavior will work on mobile web and native.

Web Navigation with a Custom Navigator

Do a feasibility study into how we can achieve the current “three pane view” with a stack navigator only. This is something I’ll probably bring up to Margelo, Software Mansion, and the react navigation discord to get some thoughts on before going too far down the rabbit hole.

parasharrajat commented 1 year ago

I would suggest testing the web and mobile web flow on the firefox browser. I have noticed that issues are easily reproducible on firefox. e.g, opening a chat via Newchat page does not always open the chat, the user is immediately navigated back to the previously active chat.

marcaaron commented 1 year ago

Thanks, will keep that in mind.

JmillsExpensify commented 1 year ago

@marcaaron is prepping a discussion for open source to keep this moving forward!

marcaaron commented 1 year ago

Continuing conversation here -> https://expensify.slack.com/archives/C01GTK53T8Q/p1667940831213829 🚀

marcaaron commented 1 year ago

I also did a study into using a custom navigator on web today with the default StackRouter. It works pretty good, but curious to get some more feedback on it as the main web example I've seen floating around uses a drawer navigator.

There also does seem to be some inconsistencies with browser history still when using the StackRouter - though it is much more predictable than the drawer routing and I think will eliminate a good amount of the bugs we're seeing.

I think any other edge cases or "new bugs" are good thing to save for the "detailed" where we can document things we are running into and why they happen and then loop in some experts to validate the design and propose fixes.

parasharrajat commented 1 year ago

The advantage of a Custom navigator could be to get rid of batch navigations. Currently, our DeprecatedCustomActions trigger multiple navigation actions.

For example,

when a user creates a new chat, the First RHN stack is popped out. then another navigation reset action is triggered to open the chat. Navigation is an expensive operation. Running two will put a load on the js thread. I think this is the reason why New Chat sometimes does not work on firefox.

AFAIK, we can calculate the end state and fire the navigation action once via the custom navigator.

marcaaron commented 1 year ago

Good thoughts @parasharrajat, thanks! I'd like to keep the conversation here contained to high-level updates. Those ideas would be great to bring up in the Slack channel.

marcaaron commented 1 year ago

Left a summary on where this is at in #expensify-open-source today.

It seems like the two things left to resolve are:

Should UP and DISMISS actions add items to the navigation history?
Should we be able to go “forward” on native?

I think once we have some clear direction there we can put this down into a doc and send out for feedback.

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @puneetlath, @marcaaron Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 1 year ago

Still waiting for more feedback to trickle in here so I can get started on the high level doc.

marcaaron commented 1 year ago

Conversations are mostly wrapped up. HL Draft is in progress here -> https://docs.google.com/document/d/1cQ87HcDA9RrmBM4ch7PmlNi9N9ZP8abQuPF0gKYrxDE

marcaaron commented 1 year ago

Made this official with a WN post and sent email to strategy. HL doc is basically ready.

melvin-bot[bot] commented 1 year ago

:wave: Hello Generalist Track Team - you have been assigned to review this High Level Design Doc. Please, treat this as a priority. Review this Stack Overflow for some tips on reviewing a design doc. Once you are done, simply press the Add "Reviewed Doc" comment button in the right hand side K2 pannel or follow these instructions.