johnpatrickmorgan / FlowStacks

FlowStacks allows you to hoist SwiftUI navigation and presentation state into a Coordinator
MIT License
854 stars 66 forks source link

Manage navigation manually #24

Closed josephktcheung closed 2 years ago

josephktcheung commented 2 years ago

This PR:

This addresses issue #22

iOS:

https://user-images.githubusercontent.com/4270232/165051259-680d3c3e-26fb-4ba4-9641-5acd3487295e.mp4

macOS:

https://user-images.githubusercontent.com/4270232/165051229-a5679de8-723c-473e-86a1-b8ef3caa9207.mov

josephktcheung commented 2 years ago

@johnpatrickmorgan not sure if manualNavigation is a good name, or if I should reuse embedInNavigationView boolean such that setting it to false would mean manualNavigation is true, so that there's no need for a new boolean.

johnpatrickmorgan commented 2 years ago

Thanks for raising this PR. This gives me a much clearer idea of the behaviour you wanted. It's a very interesting approach, and I think it fits nicely with some changes that I'm investigating for issue #21: I think the ability to customize the transition when 'manually navigating' would be very useful. If I can make the changes as I hope, it would allow for an API something like this:

routes.replaceCurrentScreen(with: .home, transition: .push)

Which would swap out the current screen for .home, animating the change with the provided transition. But, in contrast to your proposed changes, it would not maintain a history of previous screens so that the transition could later be wound back with routes.goBack(). I can see now how in some cases maintaining history would be desirable, so I think there's room for both styles to be supported.

The changes I'm looking into making for #21 involve a pretty major rewrite of Node, so I don't think your proposed changes could be merged at the moment, but I'd like to try to incorporate your ideas into that work.

josephktcheung commented 2 years ago

Thanks for raising this PR. This gives me a much clearer idea of the behaviour you wanted. It's a very interesting approach, and I think it fits nicely with some changes that I'm investigating for issue #21: I think the ability to customize the transition when 'manually navigating' would be very useful. If I can make the changes as I hope, it would allow for an API something like this:

routes.replaceCurrentScreen(with: .home, transition: .push)

Which would swap out the current screen for .home, animating the change with the provided transition. But, in contrast to your proposed changes, it would not maintain a history of previous screens so that the transition could later be wound back with routes.goBack(). I can see now how in some cases maintaining history would be desirable, so I think there's room for both styles to be supported.

The changes I'm looking into making for #21 involve a pretty major rewrite of Node, so I don't think your proposed changes could be merged at the moment, but I'd like to try to incorporate your ideas into that work.

@johnpatrickmorgan thanks for the review. Since you're rewriting Node, I'll keep this PR open until the feature is implemented in your next update.

Here I list out what I hope the next FlowStacks / TCACoordinators can achieve:

Problem:

  1. SwiftUI's native navigation flow (NavigationView, .sheet or .cover) has limited support in macOS (e.g. no StackNavigationViewStyle)
  2. Even if SwiftUI later provides better / full support in macOS's navigation flow, library user may still want to roll out their own navigation flow to meet their own app specification (e.g. an app that uses custom hamburger menu to navigate different screen, overlaying some screen on top while not using .sheet)

How FlowStacks can solve the above problems:

  1. Provide a way to allow user to define their own navigation flow, i.e. delegate the responsibility of screen transition to user, if they find that SwiftUI's native navigation is not suitable
  2. Route stack should be able to mix native navigation and 'manual' navigation implemented by the user seamlessly
    (N)
    N1 -> N2 // N stands for native navigation

    A user can push a screen that uses 'manual' navigation if they specify it in the .push method

    (N)                         // N stands for native navigation
    N1 -> N2 -> (M)  // M stands for 'manual' navigation
                        M1

    A user can further push any screen without specifying if the last one is native or not, the library can infer it based on the last element's attribute

    (N)                         // N stands for native navigation
    N1 -> N2 -> (M)  // M stands for 'manual' navigation
                        M1 -> M2 -> (N)
                                               N1 -> N2 ....
  3. User can set their own transition style to any 'manual' navigation route. The library can provide some sensible default like those in #21
  4. The library maintains history of the route stack (be it native or 'manual' or a mix of the two)

Let's see if the above points make sense.

Besides, if we take a step back, navigation boils down to transition of one screen to another. Hence no matter how deep one's route stack is, when one pushes / presents / overlays a new screen, what the app user sees is the interaction between current screen and next screen. And this is the gist of this PR as well.

johnpatrickmorgan commented 2 years ago

Just a quick update: the latest version of SwiftUI allows stack-based navigation on macOS, albeit only for version 13 onwards, so there is a bit less motivation for this sort of change. I also realised that this approach differs from other forms of navigation in that moving back to a previous screen recreates the screen, re-initialising its state, which might be unexpected.

josephktcheung commented 2 years ago

Thanks for taking a look into this @johnpatrickmorgan, I will close the PR.