Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage navigation manually #24

Conversation

josephktcheung
Copy link

@josephktcheung josephktcheung commented Apr 25, 2022

This PR:

  • Add manualNavigation to Route that allows user to create their own navigation flow without using Apple's native NavigationView
  • Add new example screen Manual to demonstrate using manualNavigation in both iOS and macOS

This addresses issue #22

iOS:

Simulator.Screen.Recording.-.iPad.9th.generation.-.2022-04-25.at.16.28.57.mp4

macOS:

Screen.Recording.2022-04-25.at.4.30.02.PM.mov

@josephktcheung
Copy link
Author

@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
Copy link
Owner

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
Copy link
Author

josephktcheung commented Apr 27, 2022

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 ....
  1. User can set their own transition style to any 'manual' navigation route. The library can provide some sensible default like those in [Feature Request] replaceStack(with routes: [Route<Screen>], animated: Bool = true) #21
  2. 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
Copy link
Owner

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants