Auto populate nav bar title and previous from page route by xster · Pull Request #19637 · flutter/flutter

xster · 2018-07-21T03:15:40Z

Diffbases #19729
xster/flutter@value_listenable_builder...xster:nav-bar-title-2

First step for #9446

xster · 2018-07-21T03:18:12Z

Is something like this ok for now until your new API or do you prefer changing didPush's signature (breaking API) or add something new?

yeah it's fine

xster · 2018-07-21T03:20:45Z

I couldn't quite tell from your PR. Is the convenience home: argument on MaterialApp etc going away and everything's going to be specified via Page or wtv we're going to end up calling it or is there still one exception dead-simple API for providing a home widget (to associate a default title with)?

all the major stuff (including home) will be backwards compatible, though i am intending there to be an alternative to them that doesn't require that you use them even in the simple case.

Hixie · 2018-08-01T23:16:03Z

  /// If true and [leading] is null, automatically try to deduce what the [leading]
  /// widget should be. If [leading] widget is not null, this parameter has no effect.
  ///
+  /// Specifically [CupertinoNavigationBar] would:


i think maybe "will" would be better than "would" here

Hixie · 2018-08-01T23:16:49Z

+  /// 1. Show a 'Close' button if the current route is a `fullscreenDialog`.
+  /// 2. Show a back chevron with [previousPageTitle] if [previousPageTitle] is
+  ///    not null.
+  /// 3. If the current route is a [CupertinoPageRoute] and the previous route


1 and 2 have the form "show ... if", but 3 has the form "if ... show", which is weird

Hixie · 2018-08-01T23:20:08Z

@@ -202,17 +257,19 @@ class CupertinoSliverNavigationBar extends StatelessWidget {
  /// The [largeTitle] argument is required and must not be null.
  const CupertinoSliverNavigationBar({


consider using macros for the docs on this class if they're identical to the other one

Made templates for the ones that are mostly duplicate

Hixie · 2018-08-01T23:20:45Z

+            left: 0.0,
+            right: 0.0,
+            bottom: 0.0,
+            child: new ClipRect(


can we avoid the cliprect? we're on a mission to remove all clips. they are too expensive.

not really :S

Unfortunately GitHub doesn't show code moves. I just moved this block so this file flows better.

The large title actually needs to clip. I can't just hide it behind other things because the thing on top of it is translucent.

Hixie · 2018-08-01T23:21:35Z

+}) {
+  // Auto use the CupertinoPageRoute's title if middle not provided.
+  if (title == null
+      && automaticallyImplyTitle


usually we try to line up the expressions so the && would be on the end of the last line (it works better they way you've done it here when using return)

I don't have a strong opinion either way. Dartfmt seems to place them at the end. The end it is then.

Hixie · 2018-08-01T23:22:24Z

+  if (title == null
+      && automaticallyImplyTitle
+      && currentRoute is CupertinoPageRoute
+      && currentRoute.title?.isNotEmpty == true) {


why is an empty string special? Generally we make "null" special.

Nothing in particular. Changed to null.

Hixie · 2018-08-01T23:22:32Z

+      && currentRoute.title?.isNotEmpty == true) {
+    return new Text(currentRoute.title);
+  } else {
+    return title;


return after else

Hixie · 2018-08-01T23:23:17Z

+  // Auto use the CupertinoPageRoute's title if middle not provided.
+  if (title == null
+      && automaticallyImplyTitle
+      && currentRoute is CupertinoPageRoute


it's a pity this class has hard-coded knowledge of CupertinoPageRoute but i don't have a better solution

Ya, as long as 'title' isn't a generic ModalRoute concept. We can introduce more abstraction like RouteTitleProvider.of(route) and have CupertinoPageRoute implement it but I think it's probably ok that the route and the nav bar is a bound concept like the uinavigationcontroller and the uinavigationitems inside uiviewcontrollers are bound concepts.

yeah i think it's fine as you have it, just a minor blemish on an otherwise nice design.

Hixie · 2018-08-01T23:24:35Z

+      );
+    } else {
+      return const SizedBox(height: 0.0, width: 0.0);
+    }


lots of returns-after-else in the last 30 lines

Done. I left the if-else if-else at the end which seems more compact than a series of exits

Hixie · 2018-08-01T23:25:54Z

+    } else if (route is CupertinoPageRoute<dynamic>) {
+      final CupertinoPageRoute<dynamic> cupertinoRoute = route;
+      return new ValueListenableBuilder<String>(
+        valueListenable: cupertinoRoute.previousTitle,


this will always be a frame behind.

I'm not super sure I get this one. Isn't the route push/replace/etc -> didChangePrevious -> ValueNotifier.set -> notifyListeners -> setState chain all happening one frame before the widget being first built?

you're right, i was wrong. for some reason i was thinking the listenable interface notified on microtasks, but that's streams, not listenables.

Added some code comments and a test

Hixie · 2018-08-01T23:28:53Z

+/// See also:
+///
+///  * [AnimatedBuilder], which invokes a builder on [Animation] ticks rather
+///    than a [ValueListenable] changing value.


Specifically, this seems wrong. AnimatedBuilder doesn't invoke things based on Animation ticks, it's in fact more general than this class. It ticks on any Listenable.

(this comment was meant to be on the other PR that includes this code)

Right. Changed (in the other PR)

Hixie · 2018-08-15T20:40:33Z

Looks like you have a merge error here (see e.g. the top change on your PR).

xster · 2018-08-15T21:33:13Z

Cleaned up the const lints

Hixie · 2018-08-23T23:18:54Z

modulo the comments on #20322

xster added the work in progress; do not review label Jul 21, 2018

googlebot added the cla: yes label Jul 21, 2018

xster commented Jul 21, 2018

View reviewed changes

xster force-pushed the nav-bar-title-2 branch 2 times, most recently from dfa8497 to 4e93955 Compare July 24, 2018 22:44

xster removed the work in progress; do not review label Jul 25, 2018

xster force-pushed the nav-bar-title-2 branch from 044abba to f5d2682 Compare July 25, 2018 01:41

xster requested review from Hixie and matthew-carroll July 25, 2018 01:41

zoechi mentioned this pull request Jul 25, 2018

AnimatedBuilder should be renamed #19255

Closed

xster force-pushed the nav-bar-title-2 branch from f5d2682 to 0649cd9 Compare July 27, 2018 21:25

Hixie reviewed Aug 1, 2018

View reviewed changes

xster mentioned this pull request Aug 8, 2018

Cupertino nav bar transitions between routes #20322

Merged

xster force-pushed the nav-bar-title-2 branch 3 times, most recently from d112f5b to 55fe8ee Compare August 10, 2018 17:38

xster force-pushed the nav-bar-title-2 branch 2 times, most recently from f1f3866 to ed31d97 Compare August 15, 2018 21:31

xster added 11 commits August 15, 2018 21:03

Auto-built middle and largeTitle from scaffold and route title

405089b

Auto populate nav bar title and previous from page route

fd18091

API for providing a custom previous title

479c3b8

Handle previous titles that are too long

f3f933f

Get previous route changes to update the current page via widgets

9be34ee

update valuewidgetbuilder

7c1bdde

Docs and more tests

306a0e3

accessibility

c7da78c

Extract out more reusable sub-widgets from the nav bar widget

4ee104f

review

12b465a

Re-merge unnecessary const lints

703c336

xster force-pushed the nav-bar-title-2 branch from ed31d97 to 703c336 Compare August 16, 2018 04:04

xster added the c: API break Backwards-incompatible API changes label Aug 22, 2018

xster merged commit c418b2f into flutter:master Aug 24, 2018

xster deleted the nav-bar-title-2 branch August 24, 2018 01:24

github-actions Bot locked as resolved and limited conversation to collaborators Aug 10, 2021

		@@ -202,17 +257,19 @@ class CupertinoSliverNavigationBar extends StatelessWidget {
		/// The [largeTitle] argument is required and must not be null.
		const CupertinoSliverNavigationBar({

Conversation

xster commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Aug 15, 2018

Uh oh!

xster commented Aug 15, 2018

Uh oh!

Hixie commented Aug 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xster commented Jul 21, 2018 •

edited

Loading