Skip to content

feat: added custom padding in PopupMenuButton#96657

Merged
darrenaustin merged 3 commits intoflutter:masterfrom
arafaysaleem:popup-menu-custom-padding
Jan 14, 2022
Merged

feat: added custom padding in PopupMenuButton#96657
darrenaustin merged 3 commits intoflutter:masterfrom
arafaysaleem:popup-menu-custom-padding

Conversation

@arafaysaleem
Copy link
Contributor

@arafaysaleem arafaysaleem commented Jan 14, 2022 •
edited
Loading

Provides ability to change PopupMenuButton popup menu padding. Credits to @Moluram for the original PR #81996 .

Fixes #57110.

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 flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 14, 2022
@arafaysaleem arafaysaleem mentioned this pull request Jan 14, 2022
Closed
8 tasks
darrenaustin
darrenaustin suggested changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arafaysaleem thanks for the contribution! Nice work. Just a minor comment below.

@darrenaustin
Copy link
Contributor

darrenaustin commented Jan 14, 2022

Also can we credit @Moluram in the description, as I believe this was based on their PR #81996 which we took too long to review.

gspencergoog
gspencergoog reviewed Jan 14, 2022
View reviewed changes
@darrenaustin
Copy link
Contributor

darrenaustin commented Jan 14, 2022

It also appears to have some merge conflicts with the current master branch.

@arafaysaleem
Copy link
Contributor Author

arafaysaleem commented Jan 14, 2022

It also appears to have some merge conflicts with the current master branch.

Coincidentally, somebody seems to have added a new feature to the exact same files :p. I'll resolve the conflict and the docs, then send an updated PR.

@darrenaustin
Copy link
Contributor

darrenaustin commented Jan 14, 2022

Thanks. Looks like there are some trailing spaces that the analyzer is complaining about (see the analyze-linux check for details).

@arafaysaleem
Copy link
Contributor Author

arafaysaleem commented Jan 14, 2022

Thanks. Looks like there are some trailing spaces that the analyzer is complaining about (see the analyze-linux check for details).

@darrenaustin all checks passed 👍

gspencergoog
gspencergoog approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

darrenaustin
darrenaustin approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to me too. Thx to both you and @Moluram for contributing to this.

@darrenaustin darrenaustin merged commit 49c5871 into flutter:master Jan 14, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
@engine-flutter-autoroll
49c5871 feat: added custom padding in PopupMenuButton (flutter/flutte…
829ea5c 
…r#96657)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
@engine-flutter-autoroll
49c5871 feat: added custom padding in PopupMenuButton (flutter/flutte…
6286e1b 
…r#96657)
gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Jan 18, 2022
@gspencergoog
Revert "feat: added custom padding in PopupMenuButton (flutter#96657)"
9217791 
This reverts commit 49c5871 because it breaks golden tests in Google testing.
@gspencergoog gspencergoog mentioned this pull request Jan 18, 2022
Merged
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
@engine-flutter-autoroll
49c5871 feat: added custom padding in PopupMenuButton (flutter/flutte…
8965a35 
…r#96657)
fluttergithubbot pushed a commit that referenced this pull request Jan 18, 2022
@gspencergoog
Revert "feat: added custom padding in PopupMenuButton (#96657)" (#96781)
34b8cef
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022
@engine-flutter-autoroll
49c5871 feat: added custom padding in PopupMenuButton (flutter/flutte…
b21c5ae 
…r#96657)
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@arafaysaleem @clocksmith
feat: added custom padding in PopupMenuButton (flutter#96657)
cc8f50c
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@gspencergoog @clocksmith
Revert "feat: added custom padding in PopupMenuButton (flutter#96657)" (
efcf80e 
flutter#96781)
@ronytesler
Copy link

ronytesler commented May 24, 2023

I don't see the changes in master or stable.

@mikedigit
Copy link

mikedigit commented Jun 19, 2023

It appears that a regression occured here and we lost the content of this PR which is nowhere to be found anymore.
@darrenaustin @gspencergoog could you take a look at this and maybe merge this back to master ?

@gspencergoog
Copy link
Contributor

gspencergoog commented Jul 5, 2023

Yes, it was reverted in #96781 because it seems to add extra padding around the top and bottom of the button that didn't used to be there.

cc @arafaysaleem

@ronytesler
Copy link

ronytesler commented Jul 9, 2023

So it won't be supported in the near future?

@RicharC293
Copy link

RicharC293 commented Jul 18, 2023

Any update for this?

@wwwdata
Copy link

wwwdata commented Sep 13, 2023

Hmm, why was this reverted? Now we only have this constant value of 8 and no way of changing it, See:

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

Lines 633 to 636 in f7997c5

child: SingleChildScrollView(
padding: const EdgeInsets.symmetric(
vertical: _kMenuVerticalPadding,
),

@dawoodt
Copy link

dawoodt commented Jan 2, 2024

Are there any plans to fix this behavior so that we can control the vertical padding?

@fa-fifi fa-fifi mentioned this pull request Feb 15, 2024
Closed
@bleroux bleroux mentioned this pull request Jun 19, 2024
Merged
auto-submit bot pushed a commit that referenced this pull request Jun 20, 2024
@bleroux
Make popup menu hardcoded padding configurable (#150506)
4b0c841 
## Description

This PR exposed `PopupMenuButton.menuPadding` parameter to override the hardcoded value.

Credits to @Moluram for the original PR #81996.
And to @arafaysaleem for the update in #96657.

#96657 was reverted due to a Google testing failure. `PopupMenuButton` implementation has evolved since that time so maybe we will not hit this Google testing failure. And if we do, we will try to figure out what is going on.

## Related Issue

Fixes #143512.
Fixes #57110

## Tests

Adds 2 tests, updates several tests.
sigurdm pushed a commit to sigurdm/flutter that referenced this pull request Jun 26, 2024
@bleroux @sigurdm
Make popup menu hardcoded padding configurable (flutter#150506)
b6f5551 
## Description

This PR exposed `PopupMenuButton.menuPadding` parameter to override the hardcoded value.

Credits to @Moluram for the original PR flutter#81996.
And to @arafaysaleem for the update in flutter#96657.

flutter#96657 was reverted due to a Google testing failure. `PopupMenuButton` implementation has evolved since that time so maybe we will not hit this Google testing failure. And if we do, we will try to figure out what is going on.

## Related Issue

Fixes flutter#143512.
Fixes flutter#57110

## Tests

Adds 2 tests, updates several tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gspencergoog gspencergoog gspencergoog approved these changes

+1 more reviewer

@darrenaustin darrenaustin darrenaustin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Custom Padding in PopupMenuButton

8 participants

@arafaysaleem @darrenaustin @ronytesler @mikedigit @gspencergoog @RicharC293 @wwwdata @dawoodt