Add TabNavigator

Add TabNavigator

A simple container for a TabBar. It is defined by a list of
TabNavigatorViews, each of which is just a TabLabel and
the tab's content.

The content of each tab is represented by a function
that produces a Widget called "builder".

R=abarth@chromium.org, ianh@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/9e097565b717246cfcc50d88b1bf176b613081af

[hansmuller, 2015-06-26 23:49:40]

hansmuller@chromium.org changed reviewers:
+ ianh@google.com

[hansmuller, 2015-06-26 23:51:28]

Patchset #1 (id:1) has been deleted

[hansmuller, 2015-06-26 23:51:46]

hansmuller@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2015-06-26 23:51:46]

PTAL

[Hixie, 2015-06-27 00:12:39]

lgtm

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
File sky/sdk/lib/widgets/tabs.dart (right):

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:307: final Function content;
Function doesn't have a return type. Make a typedef for this (we may already
have one).

Also, call it "builder".

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:345: labels: views.map((view) =>
view.label).toList(),
Why toList()?

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:352: <Widget>[tabBar, new Flexible(child:
content)],
We don't usually bother type-annotating the children lists we pass to Flex(),
because that would be too awkward everywhere. I guess you should remove the
annotation. :-(

[abarth-chromium, 2015-06-27 00:27:35]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org, jackson@chromium.org

[abarth-chromium, 2015-06-27 00:27:36]

We might want to think about how this integrates with Navigator more generally. 
For example, do we want a route for each tab?  That would let you address each
tab with a URL.  I don't think we should block this CL on that issue, but it's
probably a conversation worth having between you and Collin.

LGTM w/ TabNavigator made stateless.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
File sky/sdk/lib/widgets/tabs.dart (right):

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:310: return content != null ? content() : null;
Should we just build unconditionally?  What does it mean for buildContent to
return null?  We don't let normal build() functions return null.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:337: onChanged(selectedIndex);
I'm not sure it makes sense to do both of these.  What component is the
authoritative source for the selected index?  The setState call implies that
this component is the authority.  However, calling onChanged and receiving
selectedIndex in the constructor implies that it's this component's parent that
is authoritative.

What's I'd recommend is making this component stateless (i.e., remove the
setState call) and make the parent component authoritative for selectedIndex.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:346: onChanged: _selectedTabIndexChanged,
If you make that change, you can just pass onChanged directly through here
without needing to hook the callback in this component.

[hansmuller, 2015-06-29 16:49:42]

Collin and I did talk about integrating this with Route and Navigator at the
outset. Will take another run at that, now that TabNavigator is more concrete.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
File sky/sdk/lib/widgets/tabs.dart (right):

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:307: final Function content;
On 2015/06/27 00:12:38, Hixie wrote:
> Function doesn't have a return type. Make a typedef for this (we may already
> have one).
> 
> Also, call it "builder".

Thanks, that's better than the names I had in mind.

We do have typedefs for Builder:

sky/sdk/lib/widgets/navigator.dart:typedef Widget Builder(Navigator navigator,
RouteBase route);
sky/sdk/lib/widgets/widget.dart:typedef Widget Builder();

I'll use the one from widget.dart however there may be an impending collision
here.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:310: return content != null ? content() : null;
On 2015/06/27 00:27:36, abarth wrote:
> Should we just build unconditionally?  What does it mean for buildContent to
> return null?  We don't let normal build() functions return null.

I suppose it could mean that there's no view for the corresponding tab. I didn't
really intend to support that case right now, so I'll just assert that builder
(nee content) and builder() are non null.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:345: labels: views.map((view) =>
view.label).toList(),
On 2015/06/27 00:12:38, Hixie wrote:
> Why toList()?

I've changed the type of TabBar.labels to Iterable<TabLabel>.

https://codereview.chromium.org/1212993003/diff/20001/sky/sdk/lib/widgets/tab...
sky/sdk/lib/widgets/tabs.dart:352: <Widget>[tabBar, new Flexible(child:
content)],
On 2015/06/27 00:12:38, Hixie wrote:
> We don't usually bother type-annotating the children lists we pass to Flex(),
> because that would be too awkward everywhere. I guess you should remove the
> annotation. :-(

OK. As you know, without the annotation code like this compiles and passes
muster with the analyzer:

new Flex(["I am not a widget"]);

[hansmuller1, 2015-06-29 17:03:39]

Committed patchset #3 (id:60001) manually as
9e097565b717246cfcc50d88b1bf176b613081af (presubmit successful).