CupertinoNavigationBar part 1 - extract common layout logic #10337
Conversation
|
|
||
| /// Widget to place at the start of the horizontal toolbar. | ||
| final Widget leading; | ||
| /// Widget to place in the middle of the horizontal toolbar, occupying |
There was a problem hiding this comment.
nit: blank line between fields
| const double _kMiddleMargin = 16.0; | ||
|
|
||
| class _ToolbarLayout extends MultiChildLayoutDelegate { | ||
| _ToolbarLayout({ this.centerMiddle }); |
There was a problem hiding this comment.
I might be wrong but it could be a bigger refactor, since the super has some non final fields.
| if (hasChild(_ToolbarSlot.leading)) { | ||
| final BoxConstraints constraints = new BoxConstraints( | ||
| minWidth: 0.0, | ||
| maxWidth: size.width / 3, // The leading widget shouldn't take up more than 1/3 of the space. |
There was a problem hiding this comment.
3.0 to avoid int/double conversion
There was a problem hiding this comment.
Ah thanks!
Should be a linter for this :)
| } | ||
|
|
||
| if (hasChild(_ToolbarSlot.middle)) { | ||
| final double maxWidth = math.max(size.width - leadingWidth - trailingWidth - _kMiddleMargin * 2, 0.0); |
| expect(tester.getTopLeft(title).dx, 72.0); | ||
| // The toolbar's contents are padded on the right by 4.0 | ||
| expect(tester.getSize(title).width, equals(800.0 - 72.0 - 4.0)); | ||
| expect(tester.getSize(title).width, equals(800.0 - 4.0 - 16.0 - 56.0 - 16.0)); |
There was a problem hiding this comment.
72.0 was a bunch of stuff previously bundled together. Was a bit too implicit.
It was also previously missing a margin on the right side between the title and the actions. Readjusted slightly.
There was a problem hiding this comment.
Ah, cool.
If you're updating the actual layout algorithm as well, please also regenerate https://github.com/flutter/assets-for-api-docs/blob/master/material/app_bar.png (there's a dart file in that directory that you can drop into a flutter create app and run to generate the image, you'll need imagemagick and optipng).
There was a problem hiding this comment.
You could improve this further by explaining what the 16s and the 56 mean (you could even use kToolbarHeight where that's appropriate).
800.0 - 4.0 - 16.0 - 56.0 - 16.0 = 708
800.0 - 72.0 - 4.0 = 724
What changed?
There was a problem hiding this comment.
Added comments. Added a right side padding to the title that was missing before
|
@HansMuller should review this, but LGTM. |
| @override | ||
| bool shouldRelayout(_ToolbarLayout oldDelegate) => centerTitle != oldDelegate.centerTitle; | ||
| } | ||
| const double _kLeadingWidth = 56.0; // So it's square with kToolbarHeight. |
There was a problem hiding this comment.
The value should be kToolbarHeight
| _ToolbarLayout({ this.centerMiddle }); | ||
|
|
||
| // If false the middle widget should be left or right justified within the space | ||
| // between the leading and trailing widgets, depending on the locale's writing |
There was a problem hiding this comment.
I don't see the support for writing-direction alignment. I think what you need is a TODO above the _ToolbarSlot.middle layout section. Then document the feature when it's supported.
There was a problem hiding this comment.
Ya, premature documentation. Changed to todos
| import 'basic.dart'; | ||
| import 'framework.dart'; | ||
|
|
||
| /// [NavigationToolbar] is an unthemed layout helper to position 3 groups |
There was a problem hiding this comment.
Do we really need to say "unthemed" here? I don't think we qualify any widgets this way.
This widget doesn't position 3 groups of widgets, it positions three widgets.
There was a problem hiding this comment.
The third slot is a list, so that's not quite accurate either.
| /// Either directly use the themed app bars such as the Material [AppBar] or | ||
| /// the iOS [CupertinoNavigationBar] or wrap this widget with more theming | ||
| /// specifications for your own custom app bar. | ||
| class NavigationToolbar extends StatelessWidget { |
There was a problem hiding this comment.
I wish this had a more generic name, since it's just a layout widget, albeit a somewhat specialized one. That said, I don't have any great ideas (ThreeBar? Ugh).
There was a problem hiding this comment.
somewhat. Though I think we still want this to be opinionated about navigation toolbars in that the toggable center and paddings will be toolbar specific
|
LGTM too although I did find a few minor things to crab about. |
|
I checked it out. It wouldn't alter the output. It's just an safeguard to make sure the title didn't run into the actions. (e.g. if the 'Abc' title was just long enough to touch actions, it won't look unpadded) |
4 participants
#7508
Wasn't trivial to make the middle widget fill the rest of the space and still center itself wrt to the parent. Pull out the layout delegate thing to widget.