liveview-native / liveview-client-swiftui

MIT License
379 stars 38 forks source link

remove navigation option, make it always enabled #1110

Closed bcardarella closed 1 year ago

bcardarella commented 1 year ago

There is no good reason why navigation should be opted-into. We need to remove this as an option and have it always be enabled.

cohawk commented 1 year ago

I would assume this was opt-in due to not all applications needing Navigation, and a NavigationStack does change your View adding the NavHeader with "Back" button & "Title" etc - which not all applications need

https://github.com/liveview-native/liveview-client-swiftui/blob/57a4244dd82cbe83e3e00cc54371976085b71544/Sources/LiveViewNative/LiveViewNative.docc/Tutorials/03%20Navigation%20and%20Hero.tutorial#L50

cohawk commented 1 year ago

IMO it should be commented in the documentation for ContentView.swift with easy enable example:

//  ContentView.swift

import SwiftUI
import LiveViewNative

@MainActor
struct ContentView: View {

    @StateObject private var session = LiveSessionCoordinator(URL(string: "http://127.0.0.1:4000/")!, config: LiveSessionConfiguration(navigationMode: .enabled))

    var body: some View {
        LiveView(session: session)
    }
}

struct ContentView_Previews: PreviewProvider {
    static var previews: some View {
        ContentView()
    }
}
bcardarella commented 1 year ago

@carson-katri can we remove this requirement? I feel too many people will be tripped up by having to enable navigation.

BrooklinJazz commented 1 year ago

@cohawk alternatively we could provide an example of how to disable it. It definitely tripped me up needing to enable it.

(though I'm still not convinced that's why I encountered a "Not Connected" issue) in issue #1109

bcardarella commented 1 year ago

The real solution here is to find a way to always keep navigation enabled without any surprises. I'll discuss with the team this week on what options there are for resolving this.

My hot take is that the nav wrapper being injected into the view tree is "magic" and in Elixir we should always be steering away from implicit functionality and leaning toward explicit. If you see the different options available for NavigationMode it is a way to opt into different nav view types. Instead I suspect this would be best to put into a layout file so developers have more direct control over the viewtree.

I'm saying this without much context if this is possible or not and which edge cases may arise. Nor do I have any senes on the compleixty to move away from the current API. But I'll report back later this week.

So in this uninformed proposal your layout may look like this:

<NavigationView>
   <%= @inner_content %>
</NavigiationView>

or if you wanted a tabbed view"

<TabView>
   <%= @inner_content %>
</TabView>

This would still be an opt-in but it is going to be more familiar to LiveView devs

carson-katri commented 1 year ago

I'm not sure if this is possible or not yet. I think a simpler method for now would be to make .enabled the default value for all LiveSessionConfiguration instances, keep the existing options, and we can work on designing a server-driven API later.

bcardarella commented 1 year ago

@carson-katri can you provide more background on what would be blocking this for now and what would be available in the future to enable this?

carson-katri commented 1 year ago

@bcardarella There isn't anything blocking it afaict, but making enabled the default is very simple and would remove any confusion around navigation before ElixirConf. I don't think there is enough time to change that API before then.

bcardarella commented 1 year ago

@carson-katri ah ok, let's do that then and we can move towards the more explicit form post-conf