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

Stinsen v2 suggestions #29

Open
jblanco-applaudostudios opened this issue Sep 30, 2021 · 5 comments
Open

Stinsen v2 suggestions #29

jblanco-applaudostudios opened this issue Sep 30, 2021 · 5 comments

Comments

@jblanco-applaudostudios
Copy link

jblanco-applaudostudios commented Sep 30, 2021

Following the conversation on #28

I think that SplitView is a really common use case, I would love to see it built into Stinsen by default! 🥳 😄 I have been playing with the new version and I really like the new routing system and ways to route. These are the suggestions that I've come up with when using v2:

  • Coordinators that conform to NavigationViewCoordinatable, should already be of type NavigationViewCoordinator<SomeCoordinator>

I came up with a workaround for this, maybe it is useful for Stinsen itself:

extension NavigationCoordinatable {
    func eraseToNavigationCoordinator() -> NavigationViewCoordinator<Self> {
        return NavigationViewCoordinator(self)
    }
}
  • There should be a .root method for when the Input is a complex type, for example, a tuple. I've had to use the .root(keypath:, input:, comparator:) method and default the closure to { _, _, in true} for it to work.

  • If possible, it would be good to reference the route and pass the parameters directly: .route(to: \.someDefinedRoute(id: viewmodel.id)

  • It would be good to have the TabViewRouter have a reference to the activeTab index, in case the app has custom views/events that rely on this. I'm currently sending the child as an environment object as a workaround.

@rundfunk47
Copy link
Owner

Thanks! I'll try to answer all questions and create separate issues later on for them:

  1. Creating a NavigationViewCoordinator is a bit of a hassle, I agree, but the problem is if you have a couple of NavigationCoordinatable's after eachother, then you probably don't want to use NavigationView per default since you'd end up with two Navigation Bars. So I don't think it is possible I'm afraid. But an extension might be added.
  2. This sounds like an oversight on my part, I'll check it out.
  3. You mean instead of .route(to: \.someDefinedRoute, viewmodel.id) like it is today? No, that is not possible I'm afraid. I agree the enum-syntax looked a bit better, this is an unfortunate side-effect of having the better (in my opinion!) way of routing.
  4. Definitely doable. A better way would be to have a isActive(\.someRoute)though, Ints feel a bit dirty 🤪

@LePips
Copy link
Collaborator

LePips commented Jul 25, 2022

I thought this thread would be the place to start talking about the new navigation APIs released this past WWDC for iOS 16. I think this should be addressed soon as iOS 16 is released in the coming months. I haven't personally used the APIs in beta yet but they bring a lot of native functionality for navigation and can make things a lot cleaner for us. Even with these new APIs, I still plan on using Stinsen as a general wrapper around the native navigation because of how well other Coordinator-patterns are handled by this project.

To move to it in Stinsen would probably be a great refactor of the underlying Coordinatables and route handling. It seems problematic to have a bunch of if #available(iOS 16.0, *) throughout all files so something should be decided for that. A completely separate project would probably be needed but that doesn't sound nice.

@rundfunk47
Copy link
Owner

I haven't had the chance to look at the new APIs yet, but I'm guessing making Stinsen use them would be a refactor with lots of if-available thrown around. I'll probably throw together a proof-of-concept with the same Stinsen-API as a separate project whenever I have the time.. 🙂

@LePips
Copy link
Collaborator

LePips commented Jul 27, 2022

I was probably thinking way too fast when I wrote,

It seems problematic to have a bunch of if #available(iOS 16.0, *) throughout all files

After reading an article from a former colleague about backporting, it may not be a complete overhaul to work on.

If you would like help with your proof of concept, I am more than happy to contribute.

@Faktorealchik
Copy link

Guys do we have any progress on it? @LePips @jblanco-applaudostudios

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