Skip to content

feat: added custom padding in PopupMenuButton #96657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
darrenaustin merged 3 commits into from
Jan 14, 2022
Merged

feat: added custom padding in PopupMenuButton #96657

darrenaustin merged 3 commits into from
Jan 14, 2022

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