Add padding for Navigation Bar to account for safe area by QuncCccccc · Pull Request #102419 · flutter/flutter

QuncCccccc · 2022-04-23T00:57:50Z

This PR adds paddings for Navigation Bar so that when a Android/iOS device is in landscape mode, the Navigation Bar will respect SafeAreas by default.

Fixes #101605.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

AlexV525 · 2022-04-24T17:45:16Z

packages/flutter/lib/src/material/navigation_bar.dart

+          additionalRightPadding, additionalBottomPadding),
        child: MediaQuery.removePadding(
          context: context,
          removeBottom: true,


Should we set removeLeft and removeRight here?

Looks like that padding is getting consumed above, so it should be removed for ancestors as suggested.

Thanks for the comments! Looks like SafeArea does same thing just like what Padding() and MediaQuery.removePadding() do, so I removed both and use SafeArea instead. Let me know if you have any suggestions.
https://github.com/flutter/flutter/blob/5464c5bac7/packages/flutter/lib/src/widgets/safe_area.dart#:~:text=return%20Padding(,child%3A%20MediaQuery.removePadding(

goderbauer · 2022-04-25T17:43:55Z

packages/flutter/lib/src/material/navigation_bar.dart

+          additionalRightPadding, additionalBottomPadding),
        child: MediaQuery.removePadding(
          context: context,
          removeBottom: true,


Looks like that padding is getting consumed above, so it should be removed for ancestors as suggested.

goderbauer · 2022-04-25T17:45:14Z

packages/flutter/test/material/navigation_bar_test.dart

+    expect(paddedCenter.dx, closeTo(defaultWidth / 2, precisionErrorTolerance));
+  });
+
+


nit: remove extra whitespace here.

goderbauer

LGTM with some formatting nits

goderbauer · 2022-04-28T16:41:40Z

packages/flutter/test/material/navigation_bar_test.dart


+  testWidgets('NavigationBar respects the notch/system navigation bar in landscape mode', (WidgetTester tester) async {
+    const double safeAreaPadding = 40.0;
+    Widget _navigationBar() {


nit: remove underscore for local functions.

goderbauer · 2022-04-28T16:44:29Z

packages/flutter/test/material/navigation_bar_test.dart

+    expect(rightPaddedCenter.dx, closeTo((defaultWidth - safeAreaPadding) / 2,
+        precisionErrorTolerance));


format nit to make this easier to read:

Suggested change

expect(rightPaddedCenter.dx, closeTo((defaultWidth - safeAreaPadding) / 2,

precisionErrorTolerance));

expect(

rightPaddedCenter.dx,

closeTo((defaultWidth - safeAreaPadding) / 2, precisionErrorTolerance),

);

Basically, the line with the opening and closing parenthesis should always have the same indentation.

goderbauer · 2022-04-28T16:45:45Z

packages/flutter/test/material/navigation_bar_test.dart

+          data: const MediaQueryData(padding: EdgeInsets.fromLTRB(
+              safeAreaPadding, 0, safeAreaPadding, safeAreaPadding)),


format nit: properties of a widget should be on their own line, e.g.

Suggested change

data: const MediaQueryData(padding: EdgeInsets.fromLTRB(

safeAreaPadding, 0, safeAreaPadding, safeAreaPadding)),

data: const MediaQueryData(

padding: EdgeInsets.fromLTRB(

safeAreaPadding, 0, safeAreaPadding, safeAreaPadding,

),

),

AlexV525 · 2022-04-28T16:54:13Z

packages/flutter/test/material/navigation_bar_test.dart

+          data: const MediaQueryData(padding: EdgeInsets.fromLTRB(
+              safeAreaPadding, 0, safeAreaPadding, safeAreaPadding)),


I would prefer:

Suggested change

data: const MediaQueryData(padding: EdgeInsets.fromLTRB(

safeAreaPadding, 0, safeAreaPadding, safeAreaPadding)),

data: const MediaQueryData(

padding: EdgeInsets.fromLTRB(

safeAreaPadding,

0,

safeAreaPadding,

safeAreaPadding,

),

),

darrenaustin

Looks good. Just a question and a couple of nits below.

darrenaustin · 2022-04-28T18:13:07Z

packages/flutter/lib/src/material/navigation_bar.dart

-      child: Padding(
-        padding: EdgeInsets.only(bottom: additionalBottomPadding),
-        child: MediaQuery.removePadding(
-          context: context,
-          removeBottom: true,


Do we know what this padding and MediaQuery were doing here? It looks like they were doing something specific with bottom padding that has been removed in this PR.

Based on my understanding, the Padding(...MediaQuery.removePadding(...)) part is just giving a SafeArea for device bottom. The code does same thing just like what SafeArea api does, so when I want to add additionalLeftPadding and additionalRightPadding, I replaced all of them with SafeArea. Please let me know if it makes sense to you:)

It does. Just wanted to make sure we knew why it was ok to remove this. Thx.

darrenaustin · 2022-04-28T18:16:13Z

packages/flutter/test/material/navigation_bar_test.dart

+      );
+    }
+
+    await tester.pumpWidget(_buildWidget(navigationBar(),));


Nit: don't need the extra ','.

darrenaustin · 2022-04-28T18:20:58Z

packages/flutter/test/material/navigation_bar_test.dart

+    expect(
+        leftPaddedCenter.dx,
+        closeTo(
+            (defaultWidth + safeAreaPadding) / 2.0,
+            precisionErrorTolerance
+        ),


nit: we usually indent only two spaces for continued lines like this. This applies to all the code below this as well.

In this case it looks like the closeTo call could also be on one line?

darrenaustin

LGTM.

…ter/flutter#102419)

Moncader · 2022-08-31T05:24:10Z

@QuncCccccc @darrenaustin
This PR has made a degradation in Flutter 3.3 where if you have an app that doesn't use a widget consuming the MediaQuery's top padding (like AppBar does), the top padding of the safe area (the notification shade area) gets added to the top of the NavigationBar making the bar very tall

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 23, 2022

QuncCccccc requested review from HansMuller and goderbauer April 23, 2022 01:03

QuncCccccc added 4 commits April 22, 2022 18:08

Added padding for left and right sides based on MediaQuery

ce40557

Added unit test

514822d

Edited test file

078b6d9

Cleaned up code

1489931

QuncCccccc force-pushed the bugfix/navigationBar-doesnt-account-for-safe-area branch from 21ddb1c to 1489931 Compare April 23, 2022 01:08

QuncCccccc requested a review from Piinks April 23, 2022 01:09

QuncCccccc marked this pull request as ready for review April 23, 2022 01:10

AlexV525 reviewed Apr 24, 2022

View reviewed changes

goderbauer reviewed Apr 25, 2022

View reviewed changes

goderbauer mentioned this pull request Apr 25, 2022

no_leading_underscores_for_local_identifiers doesn't complain about local functions dart-lang/sdk#58715

Closed

Replaced Padding() and MediaQuery.removePadding with SafeArea

bb366e5

QuncCccccc requested a review from darrenaustin April 26, 2022 16:01

goderbauer approved these changes Apr 28, 2022

View reviewed changes

AlexV525 reviewed Apr 28, 2022

View reviewed changes

made format changes

6bbc529

QuncCccccc added the waiting for tree to go green label Apr 28, 2022

darrenaustin reviewed Apr 28, 2022

View reviewed changes

QuncCccccc removed the waiting for tree to go green label Apr 28, 2022

Addressed comments

531b590

QuncCccccc added the waiting for tree to go green label Apr 28, 2022

darrenaustin approved these changes Apr 28, 2022

View reviewed changes

fluttergithubbot merged commit 57a688c into flutter:master Apr 28, 2022

engine-flutter-autoroll mentioned this pull request Apr 28, 2022

Roll Flutter from 4cea9afc1224 to 57a688c1f04d (1 revision) flutter/plugins#5439

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 28, 2022

57a688c Add padding for Navigation Bar to account for safe area (flut…

97f9aa8

…ter/flutter#102419)

engine-flutter-autoroll mentioned this pull request Apr 29, 2022

Roll Flutter from 4cea9afc1224 to 5b713147404d (4 revisions) flutter/plugins#5441

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 29, 2022

57a688c Add padding for Navigation Bar to account for safe area (flut…

6d64188

…ter/flutter#102419)

engine-flutter-autoroll mentioned this pull request Apr 29, 2022

Roll Flutter from d873157287ec to 7a74222832f8 (16 revisions) flutter/packages#1661

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 29, 2022

57a688c Add padding for Navigation Bar to account for safe area (flut…

c5e8699

…ter/flutter#102419)

QuncCccccc linked an issue Apr 29, 2022 that may be closed by this pull request

NavigationBar doesn't account for safe area #101605

Closed

QuncCccccc mentioned this pull request Apr 29, 2022

NavigationBar doesn't account for safe area #101605

Closed

egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022

Add padding for Navigation Bar to account for safe area (flutter#102419)

a5c0e84

mobihen mentioned this pull request Aug 16, 2024

[Issue]: iPhone layout on RTL UI issues jellyflix-app/jellyflix#108

Open

		expect(paddedCenter.dx, closeTo(defaultWidth / 2, precisionErrorTolerance));
		});

		expect(rightPaddedCenter.dx, closeTo((defaultWidth - safeAreaPadding) / 2,
		precisionErrorTolerance));

		data: const MediaQueryData(padding: EdgeInsets.fromLTRB(
		safeAreaPadding, 0, safeAreaPadding, safeAreaPadding)),

Conversation

QuncCccccc commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

goderbauer left a comment

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!

darrenaustin left a comment

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!

darrenaustin left a comment

Choose a reason for hiding this comment

Uh oh!

Moncader commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

QuncCccccc commented Apr 23, 2022 •

edited

Loading

QuncCccccc Apr 25, 2022 •

edited

Loading