Let translucent Cupertino bars have its scaffold children automatically pad their heights by xster · Pull Request #13194 · flutter/flutter

xster · 2017-11-24T02:50:38Z

Remove manual padding to account for obstructed area
Obstructed areas are signaled to scaffold children via MediaQuery
List/GridViews automatically consume and sliver pad MediaQuery in main axis direction

@cbracken, these are the semantic 'paddings' we talked about. Let me know if you rename them.

Next PR: #12912, use actual layout dimensions instead of self reported dimensions.

…ent nav and tab bars leave behind media query paddings in scaffolds.

cbracken · 2017-11-30T20:44:44Z

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

+/// Widget that fits in [CupertinoPageScaffold.navigationBar].
+///
+/// Must report its preferred size and whether it's opaque or translucent.
+abstract class BaseCupertinoNavigationBar extends PreferredSizeWidget {


It'd be really nice to avoid this. It feels specific enough that I can't think of a great generalised class name. It would be nice to avoid tying it too closely to CupertinoNavigationBar and instead tie it to the underlying behaviour that it represents -- are we completely hiding what's below (in which case padding is applied one way) or not, in which case it's applied another.

cbracken · 2017-11-30T20:45:34Z

LGTM for the behaviour -- discussed class naming a bit offline.

xster · 2017-11-30T21:55:26Z

Time sensitive, submitting before flutter_tools test on mac finishes. Should be ok.

xster · 2017-11-30T21:58:43Z

Last test completed

Hixie · 2017-11-30T23:06:28Z

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

+  /// The scaffold assumes the nav bar will consume the [MediaQuery] top padding.
  // TODO(xster): document its page transition animation when ready
-  final PreferredSizeWidget navigationBar;
+  final ObstructingPreferredSizeWidget navigationBar;


This is a breaking change, please make sure to announce changes like this on flutter-dev.

Hixie · 2017-11-30T23:06:50Z

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

-          removeTop: true,
+      final MediaQueryData existingMediaQuery = MediaQuery.of(context);
+
+      // TODO(https://github.com/flutter/flutter/issues/12912):


syntax is TODO(username): message, url

Hixie · 2017-11-30T23:07:41Z

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

+}
+
+/// Widget that has a preferred size and reports whether it fully obstructs
+/// widgets behind it.


please add a lot more documentation here. imagine someone has never heard of this widget and they want to know why it exists, why to use it, how to use it (both on the implementing side and the consuming side), etc.

Hixie · 2017-11-30T23:07:55Z

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

  ///
-  /// Content can slide under the [tabBar] when it's translucent.
+  /// Content can slide under the [tabBar] when it's translucent with a
+  /// [MediaQuery] padding hinting the bottom obstructed area.


Hixie · 2017-11-30T23:08:01Z

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

+    if (widget.tabBar != null) {
+      final MediaQueryData existingMediaQuery = MediaQuery.of(context);
+
+      // TODO(https://github.com/flutter/flutter/issues/12912):


todo syntax

Hixie · 2017-11-30T23:11:37Z

packages/flutter/lib/src/widgets/scroll_view.dart

+        final EdgeInsets mediaQueryVerticalPadding =
+            mediaQuery.padding.copyWith(left: 0.0, right: 0.0);
+        // Consume the main axis padding with SliverPadding.
+        effectivePadding = scrollDirection == Axis.vertical


Doesn't this just throw away the padding the user specified?

oh, i see, you only do this if padding is null. never mind.

xster · 2017-12-01T23:18:06Z

Thanks for reviewing. Follow-ups in #13310

tvolkert · 2017-12-02T04:15:22Z

This has been identified as the culprit of test failures for internal Google customers and is blocking an internal roll of Flutter. See bug #70050180

xster · 2017-12-02T04:20:40Z

Reverting. A global non-disable-able auto handling isn't flexible enough. There's a gap now for the list in the gallery's drawer demo.

…omatically pad their heights (flutter#13194)" This reverts commit 5c4ffa1.

…omatically pad their heights (#13194)" (#13317) This reverts commit 5c4ffa1.

…ly pad their heights (flutter#13194) * Let lists automatically add sliver padding from media query. Translucent nav and tab bars leave behind media query paddings in scaffolds. * tests * const lint * Rename base abstract class to generalized ObstructingPreferredSizeWidget

…ly pad their heights - second try (#13440) * Let translucent Cupertino bars have its scaffold children automatically pad their heights (#13194) * Let lists automatically add sliver padding from media query. Translucent nav and tab bars leave behind media query paddings in scaffolds. * tests * const lint * Rename base abstract class to generalized ObstructingPreferredSizeWidget * review * More docs and comments from #13317

…ly pad their heights (flutter#13194) * Let lists automatically add sliver padding from media query. Translucent nav and tab bars leave behind media query paddings in scaffolds. * tests * const lint * Rename base abstract class to generalized ObstructingPreferredSizeWidget

…omatically pad their heights (flutter#13194)" (flutter#13317) This reverts commit 5c4ffa1.

…ly pad their heights - second try (flutter#13440) * Let translucent Cupertino bars have its scaffold children automatically pad their heights (flutter#13194) * Let lists automatically add sliver padding from media query. Translucent nav and tab bars leave behind media query paddings in scaffolds. * tests * const lint * Rename base abstract class to generalized ObstructingPreferredSizeWidget * review * More docs and comments from flutter#13317

Let lists automatically add sliver padding from media query. Transluc…

a4e9a6a

…ent nav and tab bars leave behind media query paddings in scaffolds.

xster requested a review from cbracken November 24, 2017 02:50

googlebot added the cla: yes label Nov 24, 2017

xster changed the title ~~Let translucent Cupertino bars have its scaffold children automatically pad it~~ Let translucent Cupertino bars have its scaffold children automatically pad their heights Nov 24, 2017

tests

4b42a55

xster force-pushed the simple-sliver-padding branch from a18b8e7 to 4b42a55 Compare November 24, 2017 03:13

const lint

54c8ca2

cbracken reviewed Nov 30, 2017

View reviewed changes

cbracken approved these changes Nov 30, 2017

View reviewed changes

Rename base abstract class to generalized ObstructingPreferredSizeWidget

0a4ce41

xster merged commit 5c4ffa1 into flutter:master Nov 30, 2017

xster deleted the simple-sliver-padding branch November 30, 2017 21:55

Hixie reviewed Nov 30, 2017

View reviewed changes

xster added a commit to xster/flutter that referenced this pull request Dec 2, 2017

Revert "Let translucent Cupertino bars have its scaffold children aut…

48b427b

…omatically pad their heights (flutter#13194)" This reverts commit 5c4ffa1.

tvolkert pushed a commit that referenced this pull request Dec 2, 2017

Revert "Let translucent Cupertino bars have its scaffold children aut…

2d6f268

…omatically pad their heights (#13194)" (#13317) This reverts commit 5c4ffa1.

xster mentioned this pull request Dec 5, 2017

Review notes from #13194 #13310

Closed

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Revert "Let translucent Cupertino bars have its scaffold children aut…

7e6458f

…omatically pad their heights (flutter#13194)" (flutter#13317) This reverts commit 5c4ffa1.

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

Conversation

xster commented Nov 24, 2017 • 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!

cbracken commented Nov 30, 2017

Uh oh!

xster commented Nov 30, 2017

Uh oh!

xster commented Nov 30, 2017

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!

xster commented Dec 1, 2017

Uh oh!

tvolkert commented Dec 2, 2017

Uh oh!

xster commented Dec 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xster commented Nov 24, 2017 •

edited

Loading