Create a CupertinoTab to support parallel navigation trees in iOS #12130

xster · 2017-09-16T00:36:25Z

Fixes #10466
Diffbases #12106.

Also create the web of documentation between all the cupertino navigation and scaffold widgets.

xster · 2017-09-16T02:07:26Z

Added test galore

Hixie · 2017-09-20T21:05:59Z

packages/flutter/lib/src/cupertino/page_scaffold.dart

+  /// Content can slide under the [navigationBar] when they're translucent.
+  final Widget child;
+
+


nit: extraneous blank line

Hixie · 2017-09-20T21:12:00Z

packages/flutter/lib/src/cupertino/page_scaffold.dart

+    if (navigationBar != null) {
+      topPadding += navigationBar.preferredSize.height;
+      if (topPadding > 0.0)
+        topPadding += MediaQuery.of(context).padding.top;


Please add a comment here (and maybe in the docs too) explaining what this is doing and why (e.g., why is a zero-height navigation bar so substantially different than a one-pixel navigation bar).

Hixie · 2017-09-20T21:12:56Z

packages/flutter/lib/src/cupertino/page_scaffold.dart

+
+    if (navigationBar != null) {
+      stacked.add(new Align(
+        alignment: FractionalOffset.topCenter,


why Align rather than Positioned?
Also, why in the center rather than forced to the width of the box?

Using Positioned would mean the navbar doesn't affect the scaffold dimensions, which seems reasonable.

ummm, it was from the other file but not sure why I did it originally. Positioned definitely makes sense. Done

Hixie · 2017-09-20T21:14:12Z

packages/flutter/lib/src/cupertino/page_scaffold.dart

+    // The main content being at the bottom is added to the stack first.
+    stacked.add(new Padding(
+      padding: new EdgeInsets.only(top: topPadding),
+      child: child,


what size do you want to give the child?
Consider the StackFit options.

I think the default loose matches the Material scaffold's behaviour. Leaving as is

Hixie · 2017-09-20T21:15:10Z

packages/flutter/lib/src/cupertino/tab.dart

+
+/// A single tab with its own [Navigator] state and history.
+///
+/// A typical tab used as the content of each tab in a [CupertinoTabScaffold]


this seems to confuse two meaning of "tab". @HansMuller might have suggestions for names to contrast the actual tab control that brings up the page, and the view that is associated with that tab.

Ya, the names do collide...

Well I think the material widgets have precedence. Wholesale renamed CupertinoTab to CupertinoTabView to match.

So tab bar is tab bar
tab bar item is just 'tab'
and the tab pages are tab views.

Hixie · 2017-09-20T21:15:42Z

packages/flutter/lib/src/cupertino/tab.dart

+///
+///  4. Finally if all else fails [onUnknownRoute] is called.
+///
+/// These navigation properties is not shared with any sibling [CupertinoTab]


Hixie · 2017-09-20T21:16:21Z

packages/flutter/lib/src/cupertino/tab.dart

+///
+/// Be sure to acquire a [BuildContext] that belongs to the [CupertinoTab] (such
+/// as by using a [Builder]) to ensure that [Navigator.of] calls uses this
+/// [CupertinoTab]'s [Navigator] rather than that of its parent.


this sentence doesn't say when this is important. Be sure to do this... when?

Actually on more thoughts, this thing might be a trap waiting to spring. Maybe I should just let the API be a Builder instead of a Widget. Any objections?

You should consider it a rule that any time you allow a widget to have a child, it should accept any Widget at all. The way that one of the widgets here requires a CupertinoTabBar is already sketchy.

Flutter is built on the idea of unopinionated composition, meaning you can build things out of other things, and they just all fit together. The least satisfying lego bricks are the ones that only fit one other kind of lego piece. :-)

I think we crossed threads somewhere.

For the whole thing about making sure to use a builder to reference the right navigator, I just avoided that trap and made the user give a builder instead of a widget. (the builder will take any widget returned if that's what you meant)

For being able to specify any widgets, I agree. Will leave #12078 open to let the tabBar property be anything.

Hixie · 2017-09-20T21:17:32Z

packages/flutter/lib/src/cupertino/tab.dart

+    return new Navigator(
+      onGenerateRoute: _onGenerateRoute,
+      onUnknownRoute: _onUnknownRoute,
+      observers: navigatorObservers ?? const <NavigatorObserver>[], // Can't be null.


this default should be in the constructor, not here

Hixie · 2017-09-20T21:18:51Z

packages/flutter/lib/src/cupertino/tab.dart

+      return true;
+    });
+    return result;
+  }


this code duplication is unfortunate. We'll have to clean this up when we do the routes refactoring.

please file a bug with the routes label to remind me to do this

Filed #12108 but didn't really come up with a good way of doing it.

The key part is that I want public fields to have customizable dartdocs which was hard to do.

Mixins just makes all the fields getters and not settable in the mixer's constructor. It could be a class by composition. Though it makes the MaterialApp WidgetsApp interface a bit awkward this shared logic is split between 2 classes.

This whole API is going to be refactored in a few months anyway. I'll clean it up then. No worries.

Hixie · 2017-09-20T21:20:16Z

packages/flutter/lib/src/cupertino/tab_scaffold.dart

+/// ```dart
+/// new CupertinoTabScaffold(
+///   tabBar: new CupertinoTabBar(
+///     items: ...


code in "Sample code" sections must be valid

Ahhh that's what the analyzer was complaining about. I couldn't figure it out. Thanks

Hixie · 2017-09-20T21:20:42Z

packages/flutter/lib/src/cupertino/tab_scaffold.dart

+///                         navigationBar: new CupertinoNavigationBar(
+///                           middle: new Text('Page 2 of tab $index'),
+///                         ),
+///                         child: ...


Hixie · 2017-09-20T21:22:33Z

packages/flutter/lib/src/cupertino/tab_scaffold.dart

+        // our own listener to update the _currentPage on top of a possibly user
+        // provided callback.
+        child: widget.tabBar.copyWith(
+            currentIndex: _currentPage,


extraneous indentation

Hixie · 2017-09-20T21:23:38Z

packages/flutter/lib/src/cupertino/page_scaffold.dart

+
+    // The main content being at the bottom is added to the stack first.
+    stacked.add(new Padding(
+      padding: new EdgeInsets.only(top: topPadding),


if you apply the MediaQuery top padding here, you should make sure to introduce a child MediaQuery that removes that padding.

I think if there's a padding here, I'd expect its descendants to never interact with window top paddings again kinda like material scaffold's body

What if the descendant is a material Scaffold? or an AppBar? Or a PageScaffold?

We should probably have a constructor on MediaQuery that takes a context and can be told to take out one of the paddings...

like

new MediaQuery.removePadding(context, removeTop: true)

(it would be a factory constructor that grabs the ambient MediaQuery, and builds a new one that's identical except with the relevant padding(s) set to 0.0)

Did you mean if there were a page scaffold with an app bar in the app bar position and another app bar in the body position? Ya, that would be problematic.

Will make the change.

Change to zero seems like a pretty specific API though. Why not just MediaQuery.of(context).copyWith?

Actually my thing is a bit cumbersome, it'd have to kinda be

new MediaQuery( data: MediaQuery.of(context).data.copyWith( padding: MediaQuery.of(context).data.padding.copyWith( top: ...

Though @cbracken had a nice idea for a nice API that wraps all this stuff in a widget. I'll just make a MediaQuery the normal way and let Chris do the nicer API.

The SafeArea widget that @cbracken and I were talking about would also introduce padding, and would need to do the removing of the padding in a secondary MediaQuery as well. So it's actually a superset of the functionality you need here, and therefore would be easily handled by the new MediaQuery.removePadding(context, removeTop: true)-style API. But whatever floats your boat. :-)

Have to get on plane :S. Manually created a MediaQuery for now

Hixie · 2017-09-20T21:27:42Z

packages/flutter/lib/src/cupertino/tab_scaffold.dart

+    }
+
+    return new Stack(
+      children: stacked,


consider if you need to specify stackFit here, too

thanks. Expand makes sense here

Hixie · 2017-09-20T21:30:43Z

modulo comments

xster · 2017-09-20T21:39:16Z

This PR diffbases another PR #12106 that creates the 2 scaffolds. For sake of clearer history readability, I'm still gonna try to apply the review notes on the source PR and land them separately.

# Conflicts: # packages/flutter/lib/cupertino.dart # packages/flutter/lib/src/cupertino/nav_bar.dart # packages/flutter/lib/src/cupertino/page_scaffold.dart # packages/flutter/lib/src/cupertino/tab_scaffold.dart # packages/flutter/test/cupertino/scaffold_test.dart

# Conflicts: # packages/flutter/lib/cupertino.dart # packages/flutter/lib/src/cupertino/page_scaffold.dart # packages/flutter/lib/src/cupertino/tab_scaffold.dart # packages/flutter/test/cupertino/scaffold_test.dart

smkhalsa · 2018-03-23T05:32:32Z

@xster Any chance we can a Material version of parallel tabbed navigation?

xster · 2018-03-23T17:53:49Z

Unlikely since it's not part of the Material Design specs (and generally doesn't create good UX patterns with Android back buttons). It's however not too hard to create that pattern in your own code if you replicate the CupertinoTabScaffold and use a material bottom tab bar.

smkhalsa · 2018-03-23T18:37:17Z

Thanks @xster for the quick response. In case anyone else is trying to do this, per @xster's suggestion, I was able to get this working by pulling out the TabSwitchingView from the CupertinoTabScaffold source code and using it as the tab view with a Material TabBar.

HansMuller · 2018-03-23T18:37:28Z

@smkhalsa Would the BottomNaviationBar work for your app?

xster · 2018-03-23T18:44:37Z

@smkhalsa just to be very sure, when you say parallel tabbed navigation, you meant having multiple navigators with their own navigation history stack states maintained independently right? If you just want different tabs with the same navigation history stack, just use BottomNavigationBar as is.

smkhalsa · 2018-03-23T18:54:40Z

@xster correct. I need independent navigation history stacks. The approach you mentioned above seems to be working well.

smkhalsa · 2018-03-23T19:01:18Z

@HansMuller I may use a BottomNavigationBar as the tab widget. However, that alone doesn't allow me to maintain separate navigation stacks for each tab and keep each tab rendered, with inactive tabs moved offscreen. This is what Cupertino's TabSwitchingView does, and I was able to successfully repurpose it to use in Material-style tab navigation.

xster added 6 commits September 14, 2017 18:51

Refactor CupertinoScaffold

a1e66dc

Rename rootTabPageBuilder to tabBuilder

f9a865a

fix tab transparency padding

11f2019

add a CupertinoTab

c0c7ab7

Add default background color

096ed24

Add a bunch of examples

34840cc

xster requested review from cbracken and Hixie September 16, 2017 00:36

googlebot added the cla: yes label Sep 16, 2017

A bunch of tests

56de67c

xster requested a review from HansMuller September 20, 2017 20:51

Hixie reviewed Sep 20, 2017

View reviewed changes

xster added 2 commits September 20, 2017 16:34

Refactor CupertinoScaffold

fd895bf

Rename rootTabPageBuilder to tabBuilder

01f8840

xster added 7 commits September 20, 2017 16:34

fix tab transparency padding

9793fab

Add default background color

5eed623

review notes

8c5700d

fix test

1592d73

Merge remote-tracking branch 'upstream/master' into cupertino-tab

a837233

# Conflicts: # packages/flutter/lib/cupertino.dart # packages/flutter/lib/src/cupertino/page_scaffold.dart # packages/flutter/lib/src/cupertino/tab_scaffold.dart # packages/flutter/test/cupertino/scaffold_test.dart

review

8b28ec2

xster force-pushed the cupertino-tab branch from 05a64f4 to 8b28ec2 Compare September 21, 2017 02:24

xster added 2 commits September 20, 2017 19:36

Rename CupertinoTab to CupertinoTabView

fe6b721

remove final ; in sample code for analyzer

2ba33c8

xster merged commit f9e8da6 into flutter:master Sep 21, 2017

xster deleted the cupertino-tab branch September 21, 2017 04:01

smkhalsa mentioned this pull request Mar 26, 2018

Routes not persisted when switching tabs lukepighetti/fluro#39

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

		/// Content can slide under the [navigationBar] when they're translucent.
		final Widget child;

Create a CupertinoTab to support parallel navigation trees in iOS #12130

Create a CupertinoTab to support parallel navigation trees in iOS #12130

Conversation

xster commented Sep 16, 2017

xster commented Sep 16, 2017

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Sep 20, 2017

xster commented Sep 20, 2017

smkhalsa commented Mar 23, 2018

xster commented Mar 23, 2018

smkhalsa commented Mar 23, 2018

HansMuller commented Mar 23, 2018

xster commented Mar 23, 2018

smkhalsa commented Mar 23, 2018

smkhalsa commented Mar 23, 2018