-
Notifications
You must be signed in to change notification settings - Fork 29.4k
Add DropdownMenu.decorationBuilder #176264
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 DropdownMenu.decorationBuilder #176264
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces DropdownMenu.decorationBuilder, a new property that provides a more flexible way to customize the InputDecoration of the DropdownMenu. This is a significant improvement over the previous method of exposing individual decoration properties. The implementation is robust, with assertions to prevent misuse and a clean refactoring that maintains backward compatibility. The accompanying tests are thorough. My review includes a couple of minor suggestions to fix typos in the documentation and a small correction in one of the new tests.
1242969 to
bf2e959
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach! Thank you for the improvement. Overall this looks good to me:) Will take another look tomorrow.
bf2e959 to
c1fb293
Compare
c1fb293 to
5da230e
Compare
| final DropdownMenuDecorationBuilder decorationBuilder = | ||
| widget.decorationBuilder ?? _buildDefaultDecoration; | ||
| InputDecoration decoration = decorationBuilder(context, controller); | ||
| // If no suffixIcon is provided, the default IconButton is used for convenience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think if there will be a case where the user doesn't want the sufficIcon at all? I'm thinking whether we should still give it a default value when suffixIcon is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.
The property DropdownMenu.showTrailingIcon was added some time ago in #167782 for that purpose.
Because DropdownMenu.decorationBuilder is new we can decide to use a different solution. Nonetheless, If we don't add a default suffix when suffixIcon is null, most of the users will have to provide their implementation or we have to offer them an API to get the default Icon. It seems to add complexity for a use case which is somewhat specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose to always add the default icon, also with a custom decoration (when the user adds an additinal trailing icon, then keep it in a Row). And the user can still disable it via the property showTrailingIcon. That way the dev does not need to implement it, e.g. also with providing more additional trailing actions, but still has the opportunity to remove it.
That way it has a purpose in all cases, while providing maximum flexibility and minimal effort for the dev.
The other way around: in the current implementation, if the user defines its own trailing icon, the showTrailingIcon does not have any effect, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose to always add the default icon, also with a custom decoration (when the user adds an additinal trailing icon, then keep it in a Row)
Adding a Row fits specific use cases and for these use cases it might be better for us to expose the default button construction if you think this would be interesting and let users add a Row (or something else) if needed (that way they can control padding for instance or anything matching their design).
My reasonning is to remove as much as possible custom rendering logic outside of DropdownMenu. In that particular case, InputDecorator already implements APIs that comply with M3 spec (padding, defaults colors, borders, etc) and offer flexibility (InputDecoration.suffix, InputDecoration.suffixIcon, etc).
The other way around: in the current implementation, if the user defines its own trailing icon, the showTrailingIcon does not have any effect, right?
Yes. It surely should be documented (in fact the best move would probably to rename showTrailingIcon to showDefaultTrailingIcon in that case). We can also decide to ignore InputDecoration.suffixIcon if showTrailingIcon is false, it might be easier to document/understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a Row fits specific use cases and for these use cases it might be better for us to expose the default button construction if you think this would be interesting and let users add a Row
Good point.
Of course the Row was just meant as an internal helper to still be able to access the default trailing icon. But yes, it would then only serve in that (in my opinion most common) use case, and shrinks flexibility how (e.g. in which order) to use it.
I personally don't need the default trailing button, as the fuctionality can easily be recreated with the controller. Not sure if folks would notice / search for the constructor of the default icon.
Yes. It surely should be documented (in fact the best move would probably to rename showTrailingIcon to showDefaultTrailingIcon in that case). We can also decide to ignore InputDecoration.suffixIcon if showTrailingIcon is false, it might be easier to document/understand.
Sounds reasonable. Happy with both variants :)
Btw thank you for your efforts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when suffixIcon is null, most of the users will have to provide their implementation or we have to offer them an API to get the default Icon. It seems to add complexity for a use case which is somewhat specific.
I see, it makes sense to me.
5da230e to
3dac456
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for adding all the tests:)
3dac456 to
789e4ec
Compare
789e4ec to
dc77ba7
Compare
flutter/flutter@891d7d5...2d34167 2025-10-20 vegorov@google.com Cleanup create_updated_flutter_deps.py a bit (flutter/flutter#177162) 2025-10-20 jessy.yameogo@gmail.com Fixed hot reload/restart crashes after closing browser tab on web-server device (flutter/flutter#177026) 2025-10-20 engine-flutter-autoroll@skia.org Roll Skia from 0a3ace6fde82 to ed4294faecde (2 revisions) (flutter/flutter#177249) 2025-10-20 bruno.leroux@gmail.com Add DropdownMenu.decorationBuilder (flutter/flutter#176264) 2025-10-20 engine-flutter-autoroll@skia.org Roll Skia from 05e2f42f533d to 0a3ace6fde82 (1 revision) (flutter/flutter#177242) 2025-10-20 engine-flutter-autoroll@skia.org Roll Skia from 89abc5393317 to 05e2f42f533d (1 revision) (flutter/flutter#177238) 2025-10-20 koji.wakamiya@gmail.com [ios][engine] Fix autofill context cleanup and view lifecycle management (flutter/flutter#173598) 2025-10-20 rajveer0malviya@gmail.com Fix Image.network not using cache when headers are specified (flutter/flutter#176831) 2025-10-19 engine-flutter-autoroll@skia.org Roll Dart SDK from a66f334fee2a to 2cd2106f2cef (4 revisions) (flutter/flutter#177190) 2025-10-19 engine-flutter-autoroll@skia.org Roll Skia from 2d424175a481 to 89abc5393317 (1 revision) (flutter/flutter#177235) 2025-10-19 ahmedsameha1@gmail.com Make sure that a ListTile doesn't crash in 0x0 environment (flutter/flutter#176176) 2025-10-19 ahmedsameha1@gmail.com Make sure that a DropdownButton doesn't crash in 0x0 environment (flutter/flutter#174880) 2025-10-19 engine-flutter-autoroll@skia.org Roll Skia from 899155871d29 to 2d424175a481 (1 revision) (flutter/flutter#177229) 2025-10-19 engine-flutter-autoroll@skia.org Roll Skia from b864c56efb66 to 899155871d29 (1 revision) (flutter/flutter#177227) 2025-10-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from M8WT2GMY46e_0fFho... to tKrvmvTOQITL81oOC... (flutter/flutter#177223) 2025-10-19 engine-flutter-autoroll@skia.org Roll Skia from 0992b560454f to b864c56efb66 (1 revision) (flutter/flutter#177222) 2025-10-18 matt.boetger@gmail.com Fix HEIF decoding (flutter/flutter#176860) 2025-10-18 engine-flutter-autoroll@skia.org Roll Skia from 74df18176924 to 0992b560454f (1 revision) (flutter/flutter#177217) 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 louisehsu@google.com,stuartmorgan@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ion (#177488) ## Description This PR adds some documentation to `DropdownMenu.showTrailingIcon` in relation to `DropdownMenu.decorationBuilder`. ## Context See #176264 (comment) ## Tests Documentation only.
Description
This PR adds
DropdownMenu.decorationBuilder.The goal is to make
DropdownMenumore flexible.Before this PR, several fields are used by
DropdownMenuto create an innerInputDecoration. This approach has several limitations:InputDecorationhas more fields that the ones that are exposedDropdownMenumakes some choices that can't be change. Especially, it creates an IconButton (with hardcoded padding) which is passed toInputDecoration.suffixIcon. This innerIconButtonintroduces some difficulty related to focus management and UI customization.The new
DropdownMenu.decorationBuilderproperty offers users a way to take control on the innerInputDecorationin a non-breaking way.In a future PR, this property will help replacing the default
IconButton.Currently users can replace the
IconButtonusing this code sample:DropdownMenu without IconButton
Related Issue
Fixes DropDownMenu secondary trailing widget
Will help for Make DropdownMenu's trailing icon not focusable by default
Related discussions
#175847 (comment)
#175558 (comment)
Tests