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

Tabbed path builders #19

Open
ohitsdaniel opened this issue Feb 10, 2021 · 13 comments
Open

Tabbed path builders #19

ohitsdaniel opened this issue Feb 10, 2021 · 13 comments
Labels
enhancement New feature or request

Comments

@ohitsdaniel
Copy link
Contributor

We currently do not support Tabbed Navigation out of the box. Let's add a Tabbed path builder that takes a list of identified path builders.

Something along the lines of:

public extension PathBuilders {
   struct IdentifiedTab<Identifier: Hashable> {
      let id: Identifier
      let builder: PathBuilder
   }

   static func tabbed<ID: Hashable>(_ tabs: IdentifiedTab<ID>) -> some PathBuilder {
      ...
   }
}
@ohitsdaniel ohitsdaniel added the enhancement New feature or request label Feb 10, 2021
@krzysztofzablocki
Copy link

@ohitsdaniel awesome project, I want to use it in a new The New York Times product and our app will have Tab Bar as the root, any idea how on when this feature will be available or any tips how I could help get it implemented?

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented Apr 28, 2021

@krzysztofzablocki
Thanks! This is next up after providers (a StateObject for iOS13 applications).

I had a couple of ideas how to approach this:

1. Active navigation path stays linear

The current active navigation path is a list of identified screens

HomeScreen -> DetailScreen -> SettingsScreen.

So, if we had a TabBar that is able to show 2 different screens resulting in the following navigation tree:

          - HomeScreen
TabBar -|
          - DiscoverScreen

In this case, the active navigation path mentioned above would still be a valid path. Main issue with this is that switching tabs would require to 'delete' the current navigation state of the tab you previously viewed (which isn't ideal). We could keep the active navigation path of a tab as local (SwiftUI) state and recover it whenever a tab becomes active, but that seems hacky and I would like to avoid relying on onAppear.

2. Active navigation path refactored to a tree-like structure

The second approach will require some refactoring. Instead of keeping the navigation path linear, each node in the navigation path could either be a screen (single screen, leaf) or a split node (multiple screens, 1 active screen, subtree, naming TBD).

enum NavigationPathElement {
  case screen(IdentifiedScreen)
  case splitNode(nodeID: ScreenID, activeScreen: ScreenID, elements: [NavigationPathElement])
}

A split node could then be represented by a tab bar (or a split view / overlay at a later point). Each element in a split node has its own navigation state. Switching tabs will require to change the active screen id in a split node but will not change / delete any of the underlying navigation state.

Drawback to this: Deeplinking gets a bit more complicated and we need to be able to resolve linear navigation paths leading into split nodes by setting an 'default state' for tab bars and determining the currently active screen. This will make .replacePath in the Navigator.Datasource a bit more complicated, but as long as we cover that logic with tests, I'm fine with it. If the split node is not in the current, to-be-replaced path, we do not know which screens are accessible through the split node. Therefore, if we navigate to a split node via a linear path, we should replace the linear .screen(IdentifiedScreen) with a split node. To do that, we need to observe the active navigation path in the to-be-implemented TabbarView, similar to WildcardView.

@ohitsdaniel
Copy link
Contributor Author

Timeline for this: I'm pretty busy this week and will probably not find any time to work on this, but will try to make some room for this feature next week, latest on Friday next week.

@krzysztofzablocki
Copy link

definitely feels like the option 2 is the way to go, since deleting navigation path user was on in a given Tab would lead to a confusing experience.

Outside of TabBar will this API design also cover the situation where the user can have multi-column navigation design that can be found on some Apps (iPad / Mac), e.g.
[Side Nav][Detail Nav][Info Nav] ?

I think with that in mind it would cover all normal use-cases for the future

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented Apr 28, 2021

Outside of TabBar will this API design also cover the situation where the user can have multi-column navigation design that can be found on some Apps (iPad / Mac), e.g.
[Side Nav][Detail Nav][Info Nav] ?

Yes, the second approach would cover this as well, as each navigation 'pane' would then have it's own navigation state that it could update without effecting the other 'panes'. It would also enable global overlays which could be triggered from anywhere in the app, as requested in #66. For me, that's the main reason, why this refactor is necessary, as it opens many doors for future features. :)

I have to put some thinking into the interfaces and how setting an element 'active' in an element would look like. I will document my thought process in this issue and also add some documentation to the wiki, once I get to working on it. :)

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented May 7, 2021

I started looking into this today.

struct SomeTabbedView: View {
  var body: some View {
    TabView(
      selection: .constant(1),
      content:  {
        Text("Tab Content 1").tabItem { Text("Tab Label 1") }.tag(1)
        Text("Tab Content 2").tabItem { Text("Tab Label 2") }.tag(2)
      }
    )
  }
}

TabView in SwiftUI allows to pass a selection binding in the init. This allows us to tag our content with the associated screenID. When the newValue passed to the custom set closure is the currently active tab, we could even reset the navigation stack to its root, as many applications do. This is great.

enum PathElement: Hashable {
  case screen(IdentifiedScreen)
  case tabbed(TabScreen)

  var id: ScreenID {
    switch self {
    case .screen(let screen):
      return screen.id
    case .tabbed(let screen):
      return screen.id
    }
  }

  func ids() -> Set<ScreenID> {
    switch self {
    case .screen(let screen):
      return [screen.id]
    case .tabbed(let screen):
      return screen.ids()
    }
  }
  
  func replaceContent(of id: ScreenID, with newContent: AnyScreen) -> PathElement {
    // ....
  }
}

struct TabScreen: Hashable {
  struct Tab: Hashable {
    let head: PathElement
    let tail: [PathElement]
  }

  let id: ScreenID
  let activeTab: ScreenID
  let tabs: [Tab]

  func ids() -> Set<ScreenID> {
    tabs.reduce(
      Set<ScreenID>([id]), { acc, tab in
        acc.union(
          tab.tail.reduce(
            Set<ScreenID>([tab.head.id]), { acc, pathElement in
              acc.union(pathElement.ids())
            }
          )
        )
      }
    )
  }
  
  func replaceContent(of id: ScreenID, with newContent: AnyScreen) -> TabScreen {
    // ....
  }
}

I also thought about naming. A (Navigation)PathElement can either be a 'simple' screen or a 'tabbed' screen. The PathElement will act as an EitherType, defining different kinds of screens and exposing a common interface for replacing values / updating sub-paths. I decided to pivot away from the generalized solution for SplitViews / Overlays as they have a different behaviour and for example do not really have any 'non-active' screens. However, the Either type will allow us to add more allowed Screen types and define their replacement behaviour more locally instead of in one big navigator data source class.

Also: the library consumer will never need to touch this code and will instead rely goTo() to do the magic for them. 😄

@krzysztofzablocki
Copy link

Sounds reasonable to me, having extensibility to add additional types in the future 👍

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented May 7, 2021

(As stated some days ago, will use this issue to document some thoughts/problems that I run into while coming up with a solution for TabScreens :))

The NavigationPath type changes from [IdentifiedScreen] to [PathElement]. This allows to push/present a screen "on top of" a TabView while maintaining the navigation state of the individual tabs.

ComposableNavigator allows navigation via ScreenID and Screen objects. The latter is a bit more complicated, as it requires to find the last occurrence of a Screen object in a NavigationPath. I'm not 100% certain yet how goTo(screen: Screen, on: Screen) will find the 'last occurrence' of a Screen in a non-linear path. I will probably implement the following two-stage logic:

  1. If a .tabbed() element has a successor, first look into the root level path and try to find the Screen there.
  2. If the screen cannot be found in the root level path, look into the child navigation paths in inverse root-level order (back to front).

Examples

// C in root-level path
[.screen(A), .tabbed([B,C], [E, F]), .screen(C)] // initial navigation path

navigator.goTo(D(), on: C()) 

[.screen(A), .tabbed([B,C], [E, F]), .screen(C), .screen(D)] 

navigator.goTo(D(), on: F()) 

[.screen(A), .tabbed([B,C], [E, F, D]), .screen(C), .screen(D)] 

The same 'last-occurence' logic applies to goBack(to: Screen), dismiss(_ screen: Screen) and dismissSuccessor(of screen: Screen). I'm just wondering for the second case, would you expect [.screen(A), .tabbed([B,C], [E, F, D]), .screen(C), .screen(D)] or [.screen(A), .tabbed([B,C], [E, F, D])]?

@AlexisQapa
Copy link

I think one additional thing to consider is whether the tabview is embedded into the NavigationView or the opposite.
(One navigation view vs one per tab)
As one cannot hide the bottom bar on push right now in swiftui I often use a single NavigationView.

In the one NavigationView case when pushing to a route which contains another tab you must pop until the the tabview, switch the tab then push the new destination.
In the other case you can keep the navigation history and simply push.

@ohitsdaniel
Copy link
Contributor Author

This actually opens up another problem: who defines how the TabScreen is presented? We cannot assume that a TabView is always the root screen and therefore it can be presented either as a sheet or in a push. We could fallback to the active tab's presentation style, but then switching tabs could change the TabView's presentation style. 🤔

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented May 7, 2021

@AlexisQapa

As one cannot hide the bottom bar on push right now in swiftui I often use a single NavigationView.

ComposableNavigator wraps the application content view in a NavigationView in the Root view. Therefore, any Tabbar will always be embedded in a NavigationView. Tabs will be configurable to have their own 'inner' NavigationView.

[.screen(A), .tabbed([B,C], [E, F, D]), .screen(C), .screen(D)] 

This navigation path would mean that we first show A, then show a tabbed screen with two tabs, push/present C on top of the TabBar view, push/present D after C. So, your case would be covered. 🤔

@krzysztofzablocki
Copy link

I'm just wondering for the second case, would you expect
[.screen(A), .tabbed([B,C], [E, F, D]), .screen(C), .screen(D)] or
[.screen(A), .tabbed([B,C], [E, F, D])]?

This is interesting question, because in scenario 1 you wouldn't see that navigation side-effect, and in 2nd you'd basically be doing more of a replace path logic than goTo.

I can imagine something like an async effect happening in bg could trigger logic that the first scenario would make sense, for user action you probably want the 2nd option, as such it isn't really clear which one would be right so maybe give an option to that method that would control this?

I try to avoid Screen() and rely on ScreenID

@ohitsdaniel
Copy link
Contributor Author

ohitsdaniel commented May 7, 2021

I try to avoid Screen() and rely on ScreenID

This is the way

Screen-based navigation mostly exists for scenarios in which you are sure that your Screen is unique (like a HomeScreen) and you do not want to 'carry-through' the HomeScreen ID in your state. I guess, I should add a paragraph on this in the docs.

I can imagine something like an async effect happening in bg could trigger logic that the first scenario would make sense, for user action you probably want the 2nd option, as such it isn't really clear which one would be right so maybe give an option to that method that would control this?

Also thought about this. Something like goTo(_, on:, forceNavigation: Bool = false) that would be backwards-compatible.

This was referenced May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants