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

SwiftUIRouter using NavigationLink #43

Open
koraykoska opened this issue Jan 5, 2022 · 6 comments
Open

SwiftUIRouter using NavigationLink #43

koraykoska opened this issue Jan 5, 2022 · 6 comments

Comments

@koraykoska
Copy link
Contributor

We are using this library since starting the development of our app. It grew quite big and we start having smaller annoying issues that would not exist if it was for the standard NavigationLink.

I had to think about how a path-based routing approach could work with the standard NavigationLink in SwiftUI and there are a lot of things to consider, of course.

Now, before I go ahead and try to open a PR that allows using NavigationLink (optionally, or even make it generic and allow custom implementations of navigators?), I wanted to ask whether someone has experience with that.

Especially @frzi . Is there a reason why you opted for a custom implementation other than convenience? Did you try and see some problems or do you know what I should focus on when trying?

@frzi
Copy link
Owner

frzi commented Jan 5, 2022

The goal of SwiftUI Router was to provide an alternative to SwiftUI's NavigationView and NavigationLink. Never for them to work hand-in-hand together.

Both follow a different ideology: with NavigationLink (specifically the isActive: Binding<Bool> initializer) the state of the current screen is localized and the burden of deciding how and where to store the state falls on the user. The user also has to define the destination view when using a NavigationLink (the destination argument). The user also has very little say on the transition effects/animations.

SwiftUI Router, on the other hand, uses a global state (the path) and maintains it, removing the burden from the user. The user also isn't required to define the destination view in advance when using NavLink, Navigate and navigator.navigate(). You just tell it what path to navigate to. SwiftUI Router also isn't exclusively for 'fullscreen screens', it works just as well for any view that should only render for certain paths (e.g. a floating menu, an action button, an advertisement). And you are in full control of the transitions and animations (well, as far as SwiftUI allows you)

Having SwiftUI Router support NavigationView and NavigationLink would make the library far too complex for its purpose. Both code-wise and UX-wise. In this case it would probably be better to write a separate library. 😄

@Nickersoft
Copy link

@koraykoska @frzi While this makes sense, I'd have to disagree. Most if not all iOS apps use navigation of some kind, and if you decide to use this library, you'd need to completely build your own navigation bars and animations that you'd normally get for free. Especially if you are trying to adhere to Apple's Human Interface standards, out-of-the-box SwiftUI components will include the correct font sizing, typography, spacing, animations, etc.

I was about to open a RFC for the sheet/navigation support I added to my fork of this library when I found this issue. The only problem with my approach is you'd need to add a custom back button to your views that would call navigator.goBack() seeing you can't easily listen for back events. If I could get approval on the approach, I was planning on opening a PR.

Here are my changes:

Nickersoft@32a808a

All this adds is a presentationMode argument to the Route() constructor that tells it whether to open a sheet, push to the navigation view, or use the default mode of just instantly showing the view.

@koraykoska koraykoska reopened this Jan 10, 2022
@frzi
Copy link
Owner

frzi commented Jan 10, 2022

Thanks for the example.

The issue with the presentationMode idea is that it significantly changes the way a Route works based on the argument. With the .navigation value the Route now has a NavigationView as a "dependency", requiring it be somewhere higher up in the view hierarchy to work at all. The Route also changes from an inline view (a view that's rendered where it's defined) to a view that simply communicates with a NavigationView. This doesn't sit right with me. Instead of this I'd rather see a more explicit API. Maybe a separate NavigationRoute (and SheetRoute?) to better differentiate itself from a Route.

Currently I have no plans for SwiftUI Router as-is to work hand-in-hand with NavigationView and .sheet et al 😄

I agree that it's a worthy feature to explore. But this would be better left for a SwiftUI Router 2.0, with a (major?) overhaul in its API. A great opportunity to explore ways to better integrate NavigationView into SwiftUI Router. And maybe see if it could play nicely with iOS' TabView as well!

Examples of a 2.0 could be:

  • Turn Router into a wrapper around a NavigationView (hiding the UI elements by default)
  • Make Route a more abstract view/object so other libraries can utilize it and build different presentation modes around it. Keeping SwiftUI Router very basic in its functionality, but easy to expand upon.

To me adding support for NavigationView and .sheet comes as a significant change that also fundamentally changes the way you think about setting up routes and the structure of your views/code in general. Rather than adding it to SwiftUI Router as it is now I'd rather see a more streamlined API. Something that feels consistent across the board.

I'm keeping this issue open - or may turn it into a discussion, as I think it's important to keep this all in mind for future developments of SwiftUI Router. Even if I've been pretty absent from the SwiftUI scene lately 😅

@Nickersoft
Copy link

Nickersoft commented Jan 10, 2022

Hey @frzi! Thanks so much for the detailed response. I actually did almost create a separate component, but didn't want to duplicate a bunch of logic 😂.

I have a proposal, in that case: the biggest reason I forked this repo and suggested an upstream change to the API is the fact that the logic that actually performs path matching is internal to the route component. I would have just as soon created a NavigationRoute in my own codebase, but I would have had to extract a ton of logic that matches the path from the PathMatcher class to create the NavigationLink Binding.

I previously had a NavigationLink nested inside a Route, but this would sometimes create issues with animation when pushing a view onto the stack. I'd prefer to implement it how it is implemented in my fork, as it ensures the NavigationLink is always rendered and then just uses a binding to control the view.

Do you think it'd be possible to generalize the internal logic of the route matching and expose it? Then we could append the README to include an example of how you could leverage the path matching to push views via NavigationLink. If we can agree on how we should expose it then I can get a PR open.

Perhaps creating a wrapper component such as PathMatcher (changing the name of the current ObservableObject of the same name), that accepts a validator and route and will pass a RouteInformation? variable into its closure? It'd be separate from the Route view in the sense that it would still render children regardless of whether the path was matched or not, and then Route could leverage this component.

Alternatively, at a bare minimum, maybe just make the current PathMatcher public? What do you think?

@frzi
Copy link
Owner

frzi commented Jan 20, 2022

Hm I wonder whether making the PathMatcher public is enough. There are several other puzzle pieces that need to be properly set for deeper Routes to work.

Will have to give this a proper think. It's times like these I wish Swift had something like abstract classes/structs 😛

@aehlke
Copy link

aehlke commented May 27, 2022

@Nickersoft did you continue with this? I'm beginning a project now and like your ideas for this but don't know what else to use meanwhile

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

No branches or pull requests

4 participants