[NavigationDrawer] adds padding property in NavigationDrawer Widget #123961

piedcipher · 2023-04-02T09:36:51Z

Adds tilePadding property to NavigationDrawer Widget.

Without adding `tilePadding` in NavigationDrawer	With `tilePadding: EdgeInsets.all(16)` in NavigationDrawer

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.

flutter-dashboard · 2023-04-02T09:36:54Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

bernaferrari · 2023-04-02T16:04:40Z

This is kind of weird, wouldn't something like double spacer be better? Because padding affects the first and last position, you don't want the space to affect the distance between header and the first element.

cc @rydmike

piedcipher · 2023-04-02T16:12:52Z

This is kind of weird, wouldn't something like double spacer be better? Because padding affects the first and last position, you don't want the space to affect the distance between header and the first element.

cc @rydmike

I used EdgeInsetsGeometry just to provide flexibility.

piedcipher · 2023-04-03T21:25:34Z

Sorry @christopherfujino, I don't know how did you got added as reviewer. Sorry.

piedcipher · 2023-04-03T21:27:49Z

I also don't know how some of these commits by @engine-flutter-autoroll got added in my pr?!

piedcipher · 2023-04-03T21:48:01Z

Now they're gone. I suspect a previous rebase might have messed up something.

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

hannah-hyj · 2023-04-10T17:10:43Z

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

padding sounds like padding to the drawer widget, I prefer name it to tilePadding or itemPadding,

Also, you probably want to add it to navigation_drawer_theme too.

goderbauer

LGTM

werainkhatri

LGTM. thanks for another contribution!

werainkhatri · 2023-05-05T04:47:25Z

@piedcipher just an FYI, you don't need to force push (rebase or merge master) that frequently. the only need for it is when a test flakes, there's a merge conflict, or the PR is quite old (which causes few tests' keys to expire needing rebase). force pushing multiple times w/o any particular reason causes resources to spin up and execute the same tests and code resulting in wastage.

if luci-flutter is failing, just wait it out, it'll auto-update when the tree is fixed.

if Google testing fails, just wait (or ask) for a Google employee to take a look.

piedcipher · 2023-05-05T05:21:00Z

@piedcipher just an FYI, you don't need to force push (rebase or merge master) that frequently. the only need for it is when a test flakes, there's a merge conflict, or the PR is quite old (which causes few tests' keys to expire needing rebase). force pushing multiple times w/o any particular reason causes resources to spin up and execute the same tests and code resulting in wastage.

if luci-flutter is failing, just wait it out, it'll auto-update when the tree is fixed.

if Google testing fails, just wait (or ask) for a Google employee to take a look.

Oh okay, I read somewhere that if your code is behind google's code commit, it (google testing) would fail. Will take care now onwards. luci-flutter & flakes I'm aware of but I used to do this frequently for google testing only. You're right. Thanks a lot! I had this question regarding how often one should update the branch.

werainkhatri · 2023-05-05T06:05:13Z

I read somewhere that if your code is behind google's code commit, it (google testing) would fail.

i've never seen / experienced this. could you share the link of the doc you saw this in?

piedcipher · 2023-05-05T07:04:02Z

I read somewhere that if your code is behind google's code commit, it (google testing) would fail.

i've never seen / experienced this. could you share the link of the doc you saw this in?

https://github.com/flutter/flutter/wiki/Understanding-Google-Testing#when-does-a-premerge-pull-request-need-rebase

piedcipher · 2023-05-05T07:05:21Z

I reread the link again, I shouldn't have rebased this frequently but I was in wrong assumption that I need to catchup with the codebase update to make sure google testing doesn't fail.

…Widget (flutter/flutter#123961)

Manual roll Flutter from 0b65723 to 43ac23b (30 revisions) Manual roll requested by [email protected] flutter/flutter@0b65723...43ac23b 2023-05-05 [email protected] targets/web.dart - fix typo (flutter/flutter#126114) 2023-05-05 [email protected] Roll Flutter Engine from 6b467df16e11 to 758cbadfac1f (2 revisions) (flutter/flutter#126175) 2023-05-05 [email protected] add tests for dominant bottom sheet in scaffold (flutter/flutter#124472) 2023-05-05 [email protected] Added CupertinoDatepicker monthYear mode (flutter#93508) (flutter/flutter#125603) 2023-05-05 [email protected] Add `Switch.trackOutlineWidth` property (flutter/flutter#125848) 2023-05-05 [email protected] Rename iosdeeplinksettings to iosuniversallinksettings (flutter/flutter#126173) 2023-05-05 [email protected] Roll Flutter Engine from f3efe11f4449 to 6b467df16e11 (3 revisions) (flutter/flutter#126174) 2023-05-05 [email protected] improvement : removed required kotlin dependency (flutter/flutter#125002) 2023-05-05 [email protected] Fix incorrect assert hint in flutter.groovy (flutter/flutter#125283) 2023-05-05 [email protected] Update .cirrus.yml (flutter/flutter#126166) 2023-05-05 [email protected] tool: replace top-level functions with enum properties (flutter/flutter#126167) 2023-05-05 [email protected] Use direct dart API from `dart:ui_web` rather than JS shim. (flutter/flutter#123443) 2023-05-05 [email protected] Add sample code for SliverAppBar (flutter/flutter#125785) 2023-05-05 [email protected] Roll Flutter Engine from cef0e9d1a94f to f3efe11f4449 (3 revisions) (flutter/flutter#126163) 2023-05-05 [email protected] Add a ReorderableListView example with cards + cleanup existing tests (flutter/flutter#126155) 2023-05-05 [email protected] Roll Packages from 6bd59cd to a0f8fd8 (4 revisions) (flutter/flutter#126161) 2023-05-05 [email protected] Bring back the failing build_test's (flutter/flutter#126014) 2023-05-05 [email protected] [web] Use plain platform views in benchmarks (flutter/flutter#126080) 2023-05-05 [email protected] Fix Material 3 tab indicator weight and position (flutter/flutter#125883) 2023-05-05 [email protected] Roll Flutter Engine from b0f53e7751ad to cef0e9d1a94f (1 revision) (flutter/flutter#126150) 2023-05-05 [email protected] Roll Flutter Engine from 6f26066144fb to b0f53e7751ad (2 revisions) (flutter/flutter#126148) 2023-05-05 [email protected] Roll Flutter Engine from 764991e046c6 to 6f26066144fb (1 revision) (flutter/flutter#126141) 2023-05-05 [email protected] Roll Flutter Engine from e7cd29153aa9 to 764991e046c6 (1 revision) (flutter/flutter#126137) 2023-05-05 [email protected] Roll Flutter Engine from a885ed472eea to e7cd29153aa9 (1 revision) (flutter/flutter#126135) 2023-05-05 [email protected] Roll Flutter Engine from c97a0deccbc1 to a885ed472eea (1 revision) (flutter/flutter#126129) 2023-05-05 [email protected] [NavigationDrawer] adds padding property in NavigationDrawer Widget (flutter/flutter#123961) 2023-05-05 [email protected] Roll Flutter Engine from 269ce2deebeb to c97a0deccbc1 (1 revision) (flutter/flutter#126124) 2023-05-05 [email protected] Roll Flutter Engine from 4d5070672859 to 269ce2deebeb (16 revisions) (flutter/flutter#126115) 2023-05-05 [email protected] Minor fixes found while working on blankcanvas (flutter/flutter#125751) 2023-05-05 [email protected] tool-web: use ProcessUtil.run to invoke child processes (flutter/flutter#126109) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...

) Manual roll Flutter from 0b65723 to 43ac23b (30 revisions) Manual roll requested by [email protected] flutter/flutter@0b65723...43ac23b 2023-05-05 [email protected] targets/web.dart - fix typo (flutter/flutter#126114) 2023-05-05 [email protected] Roll Flutter Engine from 6b467df16e11 to 758cbadfac1f (2 revisions) (flutter/flutter#126175) 2023-05-05 [email protected] add tests for dominant bottom sheet in scaffold (flutter/flutter#124472) 2023-05-05 [email protected] Added CupertinoDatepicker monthYear mode (flutter#93508) (flutter/flutter#125603) 2023-05-05 [email protected] Add `Switch.trackOutlineWidth` property (flutter/flutter#125848) 2023-05-05 [email protected] Rename iosdeeplinksettings to iosuniversallinksettings (flutter/flutter#126173) 2023-05-05 [email protected] Roll Flutter Engine from f3efe11f4449 to 6b467df16e11 (3 revisions) (flutter/flutter#126174) 2023-05-05 [email protected] improvement : removed required kotlin dependency (flutter/flutter#125002) 2023-05-05 [email protected] Fix incorrect assert hint in flutter.groovy (flutter/flutter#125283) 2023-05-05 [email protected] Update .cirrus.yml (flutter/flutter#126166) 2023-05-05 [email protected] tool: replace top-level functions with enum properties (flutter/flutter#126167) 2023-05-05 [email protected] Use direct dart API from `dart:ui_web` rather than JS shim. (flutter/flutter#123443) 2023-05-05 [email protected] Add sample code for SliverAppBar (flutter/flutter#125785) 2023-05-05 [email protected] Roll Flutter Engine from cef0e9d1a94f to f3efe11f4449 (3 revisions) (flutter/flutter#126163) 2023-05-05 [email protected] Add a ReorderableListView example with cards + cleanup existing tests (flutter/flutter#126155) 2023-05-05 [email protected] Roll Packages from 6bd59cd to a0f8fd8 (4 revisions) (flutter/flutter#126161) 2023-05-05 [email protected] Bring back the failing build_test's (flutter/flutter#126014) 2023-05-05 [email protected] [web] Use plain platform views in benchmarks (flutter/flutter#126080) 2023-05-05 [email protected] Fix Material 3 tab indicator weight and position (flutter/flutter#125883) 2023-05-05 [email protected] Roll Flutter Engine from b0f53e7751ad to cef0e9d1a94f (1 revision) (flutter/flutter#126150) 2023-05-05 [email protected] Roll Flutter Engine from 6f26066144fb to b0f53e7751ad (2 revisions) (flutter/flutter#126148) 2023-05-05 [email protected] Roll Flutter Engine from 764991e046c6 to 6f26066144fb (1 revision) (flutter/flutter#126141) 2023-05-05 [email protected] Roll Flutter Engine from e7cd29153aa9 to 764991e046c6 (1 revision) (flutter/flutter#126137) 2023-05-05 [email protected] Roll Flutter Engine from a885ed472eea to e7cd29153aa9 (1 revision) (flutter/flutter#126135) 2023-05-05 [email protected] Roll Flutter Engine from c97a0deccbc1 to a885ed472eea (1 revision) (flutter/flutter#126129) 2023-05-05 [email protected] [NavigationDrawer] adds padding property in NavigationDrawer Widget (flutter/flutter#123961) 2023-05-05 [email protected] Roll Flutter Engine from 269ce2deebeb to c97a0deccbc1 (1 revision) (flutter/flutter#126124) 2023-05-05 [email protected] Roll Flutter Engine from 4d5070672859 to 269ce2deebeb (16 revisions) (flutter/flutter#126115) 2023-05-05 [email protected] Minor fixes found while working on blankcanvas (flutter/flutter#125751) 2023-05-05 [email protected] tool-web: use ProcessUtil.run to invoke child processes (flutter/flutter#126109) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...

…Widget (flutter/flutter#123961)

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

piedcipher requested a review from christopherfujino as a code owner April 3, 2023 18:50

christopherfujino removed their request for review April 3, 2023 18:51

HansMuller requested a review from goderbauer April 7, 2023 21:46

goderbauer reviewed Apr 7, 2023

View reviewed changes

packages/flutter/lib/src/material/navigation_drawer.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/navigation_drawer.dart Outdated Show resolved Hide resolved

hannah-hyj reviewed Apr 10, 2023

View reviewed changes

goderbauer approved these changes May 3, 2023

View reviewed changes

goderbauer requested a review from hannah-hyj May 3, 2023 12:33

werainkhatri approved these changes May 4, 2023

View reviewed changes

werainkhatri added the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2023

piedcipher added 5 commits May 5, 2023 09:56

add padding property in NavigationDrawer

ed20453

add widget-test

27f6821

improve doc comments for padding property

589b84a

rename property to tilePadding

bc04264

rename property to tilePadding

425a8e6

auto-submit bot merged commit 91a84de into flutter:master May 5, 2023

piedcipher deleted the enhancement/nav_drawer_padding branch May 5, 2023 05:41

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to 28f30377582a (13 revisions) flutter/packages#3912

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

540c601

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to 9de9a81b809e (14 revisions) flutter/packages#3913

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

55c5064

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to e24de32760bc (15 revisions) flutter/packages#3914

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

49aa57e

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to de2615462c02 (16 revisions) flutter/packages#3915

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

5f34366

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to f57d38f7f233 (17 revisions) flutter/packages#3917

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

3a2ef14

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to 042eaf6b90b3 (18 revisions) flutter/packages#3918

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

746e001

…Widget (flutter/flutter#123961)

HansMuller mentioned this pull request May 5, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll mentioned this pull request May 5, 2023

Roll Flutter from 0b657236cae4 to 201f7311f7c0 (19 revisions) flutter/packages#3919

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

d37bd6b

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 5, 2023

Manual roll Flutter from 0b657236cae4 to 201f7311f7c0 (19 revisions) flutter/packages#3920

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

b6b9e7b

…Widget (flutter/flutter#123961)

engine-flutter-autoroll mentioned this pull request May 6, 2023

Manual roll Flutter from 0b657236cae4 to 43ac23b4c7b4 (30 revisions) flutter/packages#3922

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

5a76cb2

…Widget (flutter/flutter#123961)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

7ccca66

…Widget (flutter/flutter#123961)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

f15ad76

…Widget (flutter/flutter#123961)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

b3821c8

…Widget (flutter/flutter#123961)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

91a84de [NavigationDrawer] adds padding property in NavigationDrawer …

ce62105

…Widget (flutter/flutter#123961)

[NavigationDrawer] adds padding property in NavigationDrawer Widget #123961

[NavigationDrawer] adds padding property in NavigationDrawer Widget #123961

Uh oh!

Conversation

piedcipher commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 2, 2023

Uh oh!

bernaferrari commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piedcipher commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piedcipher commented Apr 3, 2023

Uh oh!

piedcipher commented Apr 3, 2023

Uh oh!

piedcipher commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

hannah-hyj Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

hannah-hyj Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

werainkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

werainkhatri commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piedcipher commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

werainkhatri commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piedcipher commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piedcipher commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

piedcipher commented Apr 2, 2023 •

edited

Loading

bernaferrari commented Apr 2, 2023 •

edited

Loading

piedcipher commented Apr 2, 2023 •

edited

Loading

werainkhatri commented May 5, 2023 •

edited

Loading

piedcipher commented May 5, 2023 •

edited

Loading

werainkhatri commented May 5, 2023 •

edited

Loading

piedcipher commented May 5, 2023 •

edited

Loading

piedcipher commented May 5, 2023 •

edited

Loading