Skip to content
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.

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

Add padding to DropdownButton #115806

Merged
merged 7 commits into from Feb 23, 2023
Merged

Add padding to DropdownButton #115806

merged 7 commits into from Feb 23, 2023

Conversation

davidskelly
Copy link
Contributor

@davidskelly davidskelly commented Nov 22, 2022

Add a padding param to DropdownButton. The current clickable hitbox looks like this when using external padding:

Screenshot from 2022-11-21 16-19-41

This allows the hitbox to look like this:

Screenshot from 2022-11-21 16-20-02

Fixes #115793

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 Nov 22, 2022
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hey @davidskelly, welcome! Thanks for contributing!
This looks like a bug that should work automatically instead of having the user fix it through a padding parameter. Can you remove the padding parameter and just adding the padding in the widget so the space is correct?

@davidskelly
Copy link
Contributor Author

Hey @davidskelly, welcome! Thanks for contributing! This looks like a bug that should work automatically instead of having the user fix it through a padding parameter. Can you remove the padding parameter and just adding the padding in the widget so the space is correct?

Hi @Piinks -

I added some sample code to the linked issue - . But what I meant to describe was - In the first image I have a custom Container (that adds the outline) with padding (padding: EdgeInsets.symmetric(horizontal: 20)) around the DropdownButton, but the blue shows the smaller hitbox:
screenshot

Without the Container padding, it looks like this:
Screenshot from 2022-12-01 16-32-42

So to get to the padded container with the correct size hitbox, the DropdownButton needs the padding passed in:
screenshot2

@flutter-dashboard
Copy link

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.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thanks @davidskelly for updating, and sorry this one fell off my radar! Can you take a look at the failing analyzer check here?

@davidskelly
Copy link
Contributor Author

@Piinks - updated test!

@Piinks
Copy link
Contributor

Piinks commented Jan 5, 2023

This allows the hitbox to look like this:

Screenshot from 2022-11-21 16-20-02

I am still not sure why this cannot be handled automatically in the widget.

What happens if the user provides padding that exceeds the bounds of the box?

@@ -1128,6 +1130,11 @@ class DropdownButton<T> extends StatefulWidget {
/// instead.
final Color? dropdownColor;

/// Padding of this widget.
Copy link
Contributor

Choose a reason for hiding this comment

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

What part of the widget?

How is this different from the user just wrapping the items in padding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What part of the widget?

Maybe "Padding around the dropdown button" would be clearer?

How is this different from the user just wrapping the items in padding?

If an item is wrapped in padding - the LR - it produces an uneven look:
Screenshot from 2023-01-05 16-53-34

Screenshot from 2023-01-05 16-53-47

Copy link

Choose a reason for hiding this comment

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

How is this different from the user just wrapping the items in padding?

The popup menu already adds a padding to the box. If items are wrapped in a Padding, it produces a weird effect in the popup (yet, looking good on the button). On the other hand, if no Padding is added, the popup looks good, but the button does not.

I believe that adding the same padding added to the popup menu to the button is a good fix for the issue, which is what this PR does

Copy link
Contributor

Choose a reason for hiding this comment

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

This documentation is still lacking some more context from my first comment here. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a change to update the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! It isn't actually the padding around the button though, is it? It is inside of the button.
The docs should still also include an explanation of how this is different from just wrapping the button, or the content of the button, in a Padding widget. As well as, how does the padding affect the overall size of the button?

All of the things we have been discussing in this PR make for helpful documentation for developers. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more detail to the documentation.

@davidskelly
Copy link
Contributor Author

I am still not sure why this cannot be handled automatically in the widget.

You are suggesting that internally extra padding is just added - say a padding of 10 - instead of being passed as params?

The dropdown without my custom outline looks like this:
Screenshot from 2023-01-05 16-40-17

If internal padding is added, it would look something like this, which I don't think would be what most developers would want:
Screenshot from 2023-01-05 16-41-53

What happens if the user provides padding that exceeds the bounds of the box?

The padding would grow the bounds of the box- the widget would just get bigger.

@davidskelly
Copy link
Contributor Author

@Piinks is there anything else you need from me here?

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. This will need a second reviewer, I will seek one out.

@esouthren
Copy link
Contributor

The change LGTM, but I think it's worth mentioning for future readers that the custom Material 3 widget DropDownMenu was created (#116088) which solves problems like this, by adding DropdownMenuThemeData which allows a custom inputDecorationTheme.

I'd highly recommend using that widget over DropdownButton - it also works successfully in Material 2.

@esouthren esouthren self-requested a review February 13, 2023 17:45
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add padding to DropdownButton
4 participants