[swift] [rfc] Add TabBarScreen Sample #1098

JustinDSN · 2020-04-19T23:10:22Z

Adds a TabBarScreen sample with snapshot tests
Demonstrates a way to separate BarItem renderings as a separate Workflow to isolate contained tab screen from bar badge value fetch / calculations.
Note: Compose Screen concept is out of scope for this PR.

TODO & Open Questions:

Should we hang on to only the currently displayed rendering (current implementation) or should we accept an array of AnyScreens and use selected index to control display?
- Either implementation is technically correct. If we hang on to each screen, then the parent workflow would have to continue to render those workflows each render pass (just like the BackStackScreen).

Single Screen vs. Multiple Screens:
Pros for Single Screen:

You don't render screens early (eg: tabs that haven't been visited yet)

Cons for Single Screen:

View state could be would between renderings (eg: Scroll Position)

How do we want to demonstrate fetching / rendering the badge value for the Baz tab? A separate Workflow or a simple Worker that is awaited in DemoWorkflow?
- I'm leaning towards a Worker as its simpler and easier to grok, however I do like demonstrating the map rendering here. WDYT?
Determine the right height for the tab bar on all the devices, should this come in on the screen or is it an implementation detail?
- Also focus more in on the safe area insets thing.
Should we snip the ContainerView?
Any way to tighten up the API (I assume less verbosity)?

swift/Samples/TabBarContainer/DemoApp/Bar/BarScreen.swift

swift/Samples/TabBarContainer/DemoApp/Bar/BarWorkflow.swift

swift/Samples/TabBarContainer/DemoApp/Foo/FooWorkflow.swift

bencochran · 2020-04-21T00:02:58Z

swift/Samples/TabBarContainer/Sources/TabBarScreenViewController.swift

+        view.addSubview(tabBar)
+
+        addChild(contentViewController)
+        containerView.contentView.addSubview(contentViewController.view)


Why containerView instead of adding the contentViewController’s view directly?

I received feedback on my SplitScreen PR from @AquaGeek to use a container view to codify a view who sets it's subviews bounds to its frame on every layout pass. Decided to keep the pattern the same.

Can’t find that discussion from that PR. But I feel like this adds an unnecessary layer to the code.

Darn it was in person: #925 (comment)

For context, IIRC that discussion was around adding static container subviews in the split screen so that you don't have to redo all the sizing math when swapping out contents — you just hand the new content view to the appropriate container and it adds it as a subview and does a view.frame = bounds.

bencochran · 2020-04-21T00:03:58Z

swift/Samples/TabBarContainer/Sources/TabBarScreenViewController.swift

+    internal override func viewDidLayoutSubviews() {
+        super.viewDidLayoutSubviews()
+
+        let tabBarHeight: CGFloat = 83


Can we ask the tab bar its height? And how does this work across iPhone X-ish sized phones with bottom safe area insets and iPhone 8-ish ones without them?

bencochran · 2020-04-21T00:07:58Z

swift/Samples/TabBarContainer/SnapshotTests/TabBarContainerSnapshotTests.swift

+        FBSnapshotVerifyView(
+            viewController.view,
+            identifier: name,
+            suffixes: ["_64"]


What’s the _64for?

bencochran · 2020-04-21T00:09:05Z

swift/Samples/TabBarContainer/DemoApp/Foo/FooWorkflow.swift

+
+extension FooWorkflow {
+
+    enum Action: WorkflowAction {


Do we need this action?

Nope removing both in Bar and Foo.

swift/Samples/TabBarContainer/DemoApp/DemoWorkflow.swift

swift/Samples/TabBarContainer/DemoApp/Bar/BarScreen.swift

swift/Samples/TabBarContainer/DemoApp/Bar/BarBadgeWorkflow.swift

swift/Samples/TabBarContainer/Sources/TabBarScreen.swift

swift/Samples/TabBarContainer/DemoApp/Bar/BarWorkflow.swift

dhavalshreyas · 2020-04-22T02:07:45Z

swift/Samples/TabBarContainer/Sources/TabBarScreenViewController.swift

+    override internal func viewDidLoad() {
+        super.viewDidLoad()
+
+        view.addSubview(containerView)


Any reason we shouldn't use UITabBarController here?

I've seen two internal implementations that use BluePrint to render the tab bar and only accept the currently displayed screen.

This allows greater control of what the UI rendering / view behavior looks like. For this sample, I didn't want to use Blueprint, so decided to use a simple UITabBar.

The question of holding onto an array of multiple screens is a good one and should be discussed in more detail. I'd like a few more folks to weigh their opinions on this PR before we circle up as a group to decide.

swift/Samples/TabBarContainer/Sources/TabBarScreenViewController.swift

swift/Samples/TabBarContainer/DemoApp/DemoWorkflow.swift

AquaGeek · 2020-04-30T20:50:49Z

swift/Samples/TabBarContainer/DemoApp/Baz/BazBadgeWorkflow.swift

+
+extension BazBadgeWorkflow {
+
+    struct BarBadgeWorker: Worker {


Is this intentionally called Bar instead of Baz?

These names are confusing. I don't have better suggestions outside of building more "real" UI.

AquaGeek · 2020-04-30T20:55:29Z

swift/Samples/TabBarContainer/DemoApp/DemoWorkflow.swift

+            }
+
+        return TabBarContainerScreen(
+            screens: [bazScreen, fooScreen],


I'm very much of the mind that all the tabs should be rendered in each pass.

AquaGeek · 2020-04-30T21:01:26Z

swift/Samples/TabBarContainer/Sources/TabBarContainerScreen.swift

+extension BarItem {
+    public enum Badge {
+        case none
+        case value(Int)


Why a separate case for this? Why not force consumers to do the text conversion?

AquaGeek · 2020-04-30T21:10:00Z

How do we want to demonstrate fetching / rendering the badge value for the Baz tab? A separate Workflow or a simple Worker that is awaited in DemoWorkflow?

Can we use a map function to convert the Output of the hosted workflows to a badge value? e.g.:

MessageListWorkflow has an output of "unread messages," which we map to a badge value when rendering it into a tab screen.
SettingsWorkflow has an output of "available updates," which we also map to a badge value.

This way, if we reparent these workflows into a different container (e.g. hamburger menu side bar) we don't have to change their content at all.

bencochran · 2020-04-30T21:54:40Z

I’d say badge value should be part of rendering of children, not outputs (i.e. it’s their state, not events).

JustinDSN · 2020-04-30T23:16:16Z

swift/Samples/TabBarContainer/Sources/TabScreen.swift

+public struct TabScreen {
+    public let barItem: BarItem
+    public let content: AnyScreen
+    public let onSelect: (() -> ())


Snip this, and handle the selection at the parent.

JustinDSN · 2020-04-30T23:21:07Z

swift/Samples/TabBarContainer/DemoApp/Baz/BazWorkflow.swift

+// MARK: Rendering
+
+extension BazWorkflow {
+    typealias Rendering = TabScreen


This should be just BazScreen

JustinDSN · 2020-04-30T23:21:40Z

swift/Samples/TabBarContainer/DemoApp/Baz/BazWorkflow.swift

+
+    typealias State = Void
+    enum Output {
+        case tabSelected


Create an worker output for the badgeCountUpdate.

JustinDSN · 2020-05-21T23:59:43Z

I’d say badge value should be part of rendering of children, not outputs (i.e. it’s their state, not events).

@bencochran could you clarify this a little more. Do you mean that you'd rather see the rendering be a tuple of screen and badge count? Instead of using the child workflow's output when the badge count is updated?

JustinDSN · 2020-06-02T22:42:30Z

Closing for now - will re-open post repo move and when I have time to wrap up.

JustinDSN added proposal Issue or PR that proposes something and invites comment on whether or not it's a good idea. swift Affects the Swift library. labels Apr 19, 2020

JustinDSN added this to the v1.0.0 milestone Apr 19, 2020

JustinDSN requested review from bencochran, AquaGeek, amorde, asthatrivedi and dhavalshreyas April 19, 2020 23:10

JustinDSN force-pushed the jsteffen/TabBarScreen branch from 485ddad to 5947904 Compare April 19, 2020 23:29

asthatrivedi reviewed Apr 20, 2020

View reviewed changes

swift/Samples/TabBarContainer/DemoApp/Bar/BarScreen.swift Outdated Show resolved Hide resolved

asthatrivedi reviewed Apr 20, 2020

View reviewed changes

swift/Samples/TabBarContainer/DemoApp/Bar/BarWorkflow.swift Outdated Show resolved Hide resolved

asthatrivedi reviewed Apr 20, 2020

View reviewed changes

swift/Samples/TabBarContainer/DemoApp/Bar/BarWorkflow.swift Outdated Show resolved Hide resolved

asthatrivedi reviewed Apr 20, 2020

View reviewed changes

swift/Samples/TabBarContainer/DemoApp/Foo/FooWorkflow.swift Outdated Show resolved Hide resolved

JustinDSN force-pushed the jsteffen/TabBarScreen branch from 5947904 to a68dde4 Compare April 20, 2020 23:58

bencochran reviewed Apr 21, 2020

View reviewed changes

JustinDSN force-pushed the jsteffen/TabBarScreen branch 2 times, most recently from 7b1c413 to 1e74402 Compare April 22, 2020 02:00

dhavalshreyas reviewed Apr 22, 2020

View reviewed changes

swift/Samples/TabBarContainer/DemoApp/DemoWorkflow.swift Show resolved Hide resolved

JustinDSN force-pushed the jsteffen/TabBarScreen branch 2 times, most recently from 7ca7ed8 to 5a12fc5 Compare April 22, 2020 17:10

Add TabBarContainer

20298cc

JustinDSN force-pushed the jsteffen/TabBarScreen branch from 5a12fc5 to aed5b3a Compare April 30, 2020 03:10

dhavalshreyas and others added 2 commits April 29, 2020 20:29

Tabbar Brainstorming

b7f7d92

Drop container view

782c43b

JustinDSN force-pushed the jsteffen/TabBarScreen branch from aed5b3a to 782c43b Compare April 30, 2020 03:42

AquaGeek reviewed Apr 30, 2020

View reviewed changes

JustinDSN commented Apr 30, 2020

View reviewed changes

dhavalshreyas mentioned this pull request May 18, 2020

Proposal: Introduce WorkflowUICommon #1168

Open

JustinDSN closed this Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swift] [rfc] Add TabBarScreen Sample #1098

[swift] [rfc] Add TabBarScreen Sample #1098

JustinDSN commented Apr 19, 2020 •

edited

Loading

bencochran Apr 21, 2020

JustinDSN Apr 21, 2020

bencochran Apr 21, 2020

JustinDSN Apr 21, 2020 •

edited

Loading

AquaGeek Apr 30, 2020

bencochran Apr 21, 2020 •

edited

Loading

bencochran Apr 21, 2020

bencochran Apr 21, 2020

JustinDSN Apr 21, 2020

dhavalshreyas Apr 22, 2020

JustinDSN Apr 22, 2020

AquaGeek Apr 30, 2020

AquaGeek Apr 30, 2020

AquaGeek Apr 30, 2020

AquaGeek commented Apr 30, 2020

bencochran commented Apr 30, 2020

JustinDSN Apr 30, 2020

JustinDSN Apr 30, 2020

JustinDSN Apr 30, 2020

JustinDSN commented May 21, 2020 •

edited

Loading

JustinDSN commented Jun 2, 2020

[swift] [rfc] Add TabBarScreen Sample #1098

[swift] [rfc] Add TabBarScreen Sample #1098

Conversation

JustinDSN commented Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinDSN Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencochran Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AquaGeek commented Apr 30, 2020

bencochran commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinDSN commented May 21, 2020 • edited Loading

JustinDSN commented Jun 2, 2020

JustinDSN commented Apr 19, 2020 •

edited

Loading

JustinDSN Apr 21, 2020 •

edited

Loading

bencochran Apr 21, 2020 •

edited

Loading

JustinDSN commented May 21, 2020 •

edited

Loading