SRGSSR / pillarbox-apple

A next-generation reactive media playback ecosystem for Apple platforms.
https://testflight.apple.com/join/TS6ngLqf
MIT License
45 stars 6 forks source link

Improve tvOS navigation #694

Closed waliid closed 5 months ago

waliid commented 5 months ago

Description

This PR enhances navigation in the fifth main tab of the demo application.

Changes made

Result

Before After
1-examples-before 1-examples-after
2-showcase-before 2-showcase-after
3-lists-before 3-lists-after
4-search-before 4-search-after
5-settings-before 5-settings-after

Checklist

defagos commented 5 months ago

Two quick feedbacks:

defagos commented 5 months ago

Might also be handled at the cell level, e.g.:

import SwiftUI

struct ContentView: View {
    var body: some View {
        List {
            Cell(title: "A")
            Cell(title: "B")
            Cell(title: "C")
            Cell(title: "D")
            Cell(title: "E")
            Cell(title: "F")
            Cell(title: "G")
            Cell(title: "H")
            Cell(title: "I")
            Cell(title: "J")
            Cell(title: "K")
        }
    }
}

struct Cell: View {
    let title: String

    var body: some View {
#if os(tvOS)
        Button(action: {}) {
            Text(title)
                .frame(maxWidth: .infinity, alignment: .leading)
        }
        .buttonStyle(.bordered)
#else
        Text(title)
#endif
    }
}

#Preview {
    ContentView()
}

There are several button styles available, one of them might even have better behavior though this ones seems to provide the same behavior as in the Settings app

Remark: The .card style does not provide correct colors when highlighted.

defagos commented 5 months ago

Using a List moreover seems to ensure more consistent section spacing.

I also tested the demo. The behavior obtained by removing the navigation titles is definitely better. In the settings tab I think that buttons should still take the whole screen width with leading alignment, though.

I recommend you take the Settings app as goal. Even though some choices might differ (e.g. having uppercased section titles) the look & feel for a raw list should basically feel the same, from behavior, to colors or paddings.

IMHO the formalism to get a list layout on iOS and tvOS should also be identical, hiding differences between custom types (e.g. Cell or CustomList like described above), with specific differences locally handled with the preprocessor or the trick we use in Play SRG:

func constant<T>(iOS: T, tvOS: T) -> T {
#if os(tvOS)
    return tvOS
#else
    return iOS
#endif
}

I'll stop there, thanks for improving the look & feel of our tvOS demo!

waliid commented 5 months ago

I am also surprised that it doesn't work. However, it's not so much the lists that don't work, but rather the lists combined with our structure.

Here is our structure:

TabView {
    NavigationStack {
        List {
        }
    }
}

I found a solution to make it working by wrapping everything in a NavigationStack:

NavigationStack {
    TabView {
        NavigationStack {
            List {
            }
        }
    }
}

But, it won't make sense anymore when we use a design closer to Android (Card style), as we will be forced to use either a Grid or HStack.

On tvOS, we will have something like this:

NavigationStack { // No longer needed with 1. or 2.
    TabView {
        NavigationStack {
            // 1
            ScrollView {
                HStack {
                }
            }
            // OR
            // 2
            ScrollView {
                Grid {
                }
            }
        }
    }
}

As the root NavigationStack will no longer be needed, I've supposed that my fix was not really relevant. That's why I started to replace Lists with ScrollViews and HStacks to prepare for the next step.

defagos commented 5 months ago

The PR has been merged but I am not sure there will be a next step with SwiftUI, except if performance issues with nested views and grid-like structures have been fixed.

IMHO it would probably be easier to do what is required to address the current need, as the future one is likely to require a significant rewrite (and I am not sure a demo justifies this amount of work).