Added padding property in NavigationRail #69404

chinmoy12c · 2020-10-30T14:39:07Z

Description

Adds verticalSpacing and itemSpacing property in NavigationRail to provide the ability to add custom spacing between NavigationRailDestination items.

Related Issues

Fixes #69193

Tests

I added the following tests:

verticalSpacing properly applied
itemSpacing properly applied
itemSpacing overrides verticalSpacing
Extra SizedBox not added when verticalSpacing and itemSpacing are not set

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

flutter-dashboard · 2020-11-25T19:24:20Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

christopherfujino · 2020-11-30T18:07:43Z

@clocksmith can you take a look at this?

shihaohong · 2020-12-28T02:32:06Z

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

+          if (verticalSpacing != 0 || itemSpacing != 0)
+          SizedBox(
+            height: itemSpacing != 0 ? itemSpacing : verticalSpacing,
+          ),


I don't think this has the right effect. For example, in the example app in the api docs (https://master-api.flutter.dev/flutter/material/NavigationRail-class.html), you will not see any difference until you have a itemSpacing or verticalSpacing value greater than ~70-80 because this just overlays the SizedBox on top of each item/icon in the navigation rail.

Yes that's true. I guess this issue would no longer exist after making the suggested implementation of wrapping items in Padding widget.

shihaohong · 2020-12-29T03:00:15Z

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

@@ -609,6 +611,7 @@ class _RailDestination extends StatelessWidget {
  final TextStyle labelTextStyle;
  final VoidCallback onTap;
  final String indexLabel;
+  final double padding;


Why not just be an EdgeInsetsGeometry, similar to Containers? This would make the padding property more flexible and consistent with how padding is defined elsewhere.

Yes, I'll change it to accept EdgeInsetsGeometry 👍

shihaohong · 2020-12-29T03:04:08Z

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

-              splashColor: colors.primary.withOpacity(0.12),
-              hoverColor: colors.primary.withOpacity(0.04),
-              child: content,
+      child: Padding(


Also, did we discuss whether we want the padding to be surrounding the tappable InkResponse? I'm not sure what is the most useful behavior here, but we should verify whether to have Padding wrap InkResponse or vice versa.

Another optimization that can be made here is that when padding is EdgeInsets.zero or null, to completely omit including the Padding widget in the widget tree altogether. Something similar to:

if (padding != null || padding != EdgeInsets.zero) { child = Padding(padding: padding, child: child); } else { // What's there before }

Also, did we discuss whether we want the padding to be surrounding the tappable InkResponse? I'm not sure what is the most useful behavior here, but we should verify whether to have Padding wrap InkResponse or vice versa.

@shihaohong surrounding with tappable InkResponse would work, although I was thinking of a use case where the user might just want spacing between two items (like 100 or so), would it be okay to have such a long tappable area?

Another optimization that can be made here is that when padding is EdgeInsets.zero or null, to completely omit including the Padding widget in the widget tree altogether. Something similar to:

if (padding != null || padding != EdgeInsets.zero) { child = Padding(padding: padding, child: child); } else { // What's there before }

Sure, I'll make the changes :) 👍
Also, should I add a test to check that the padding is not added in case of EdgeInsets.zero.

Yes, that's a good test to add!

chinmoy12c · 2020-12-31T11:33:31Z

@shihaohong I've updated the PR. Please do tell if any changes are required regarding the tappable area wrapping padding.

Happy new year 🎉🎉

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

packages/flutter/test/material/navigation_rail_test.dart

shihaohong · 2021-01-21T08:05:08Z

packages/flutter/test/material/navigation_rail_test.dart

+    await _pumpNavigationRail(
+      tester,
+      navigationRail: NavigationRail(
+        labelType: NavigationRailLabelType.all,


Could you also add a test for NavigationRailLabelType.selected and NavigationRailLabelType.none?

Might also be worth adding a test for NavigationRailLabelType.all and NavigationRailLabelType.selected making sure that the default padding is applied (const EdgeInsets.symmetric(horizontal: _horizontalDestinationPadding) when padding is not defined.

Yeah sure I'll add a test for NavigationRailLabelType.selected and NavigationRailLabelType.none. Also, in the added test the first destination item uses default padding so it's checking for the default padding const EdgeInsets defaultPadding = EdgeInsets.symmetric(horizontal: 8.0);.

shihaohong

Very close, thanks for these updates. Just needs a few more tests to make sure the behavior is as expected and that no regressions occur.

shihaohong

LGTM

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 30, 2020

google-cla bot added the cla: yes label Oct 30, 2020

chinmoy12c force-pushed the issue_69193 branch 3 times, most recently from 20d3f13 to 5e13453 Compare October 30, 2020 15:24

HansMuller requested a review from clocksmith November 5, 2020 19:22

chinmoy12c force-pushed the issue_69193 branch from 5e13453 to 484eed7 Compare November 25, 2020 20:06

chinmoy12c force-pushed the issue_69193 branch from 484eed7 to bf69cdf Compare December 8, 2020 05:30

chinmoy12c force-pushed the issue_69193 branch from bf69cdf to 7e8eccd Compare December 26, 2020 08:33

shihaohong self-requested a review December 26, 2020 08:49

shihaohong reviewed Dec 28, 2020

View reviewed changes

chinmoy12c force-pushed the issue_69193 branch 2 times, most recently from 30d2cfe to 73df9d3 Compare December 28, 2020 10:36

chinmoy12c requested a review from shihaohong December 28, 2020 10:38

shihaohong reviewed Dec 29, 2020

View reviewed changes

chinmoy12c force-pushed the issue_69193 branch from 73df9d3 to 5d75c8c Compare December 31, 2020 11:29

chinmoy12c force-pushed the issue_69193 branch from 5d75c8c to 71e079e Compare December 31, 2020 11:51

chinmoy12c requested a review from shihaohong January 3, 2021 16:00

shihaohong reviewed Jan 6, 2021

View reviewed changes

chinmoy12c force-pushed the issue_69193 branch from 71e079e to 81f8082 Compare January 6, 2021 13:08

chinmoy12c requested a review from shihaohong January 6, 2021 13:17

shihaohong changed the title ~~Added verticalSpacing and itemSpacing property in NavigationRail~~ Added padding property in NavigationRail Jan 7, 2021

shihaohong reviewed Jan 7, 2021

View reviewed changes

packages/flutter/test/material/navigation_rail_test.dart Outdated Show resolved Hide resolved

chinmoy12c force-pushed the issue_69193 branch from 81f8082 to 71a3a0f Compare January 15, 2021 11:09

chinmoy12c requested a review from shihaohong January 15, 2021 11:10

shihaohong reviewed Jan 21, 2021

View reviewed changes

Added padding property to NavigationRail items

270c40b

chinmoy12c force-pushed the issue_69193 branch from 71a3a0f to 270c40b Compare January 22, 2021 12:33

chinmoy12c requested a review from shihaohong January 22, 2021 12:33

shihaohong approved these changes Jan 25, 2021

View reviewed changes

shihaohong added the waiting for tree to go green label Jan 25, 2021

fluttergithubbot merged commit 76d4114 into flutter:master Jan 25, 2021

Sivan22 mentioned this pull request Nov 21, 2024

בעיה בכפתור הגדרות Sivan22/otzaria#307

Open

Added padding property in NavigationRail #69404

Added padding property in NavigationRail #69404

Uh oh!

Conversation

chinmoy12c commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Nov 25, 2020

Uh oh!

christopherfujino commented Nov 30, 2020

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!

shihaohong Dec 29, 2020 • 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!

chinmoy12c commented Dec 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmoy12c commented Oct 30, 2020 •

edited

Loading

shihaohong Dec 29, 2020 •

edited

Loading