Make onChanged optional and add enabled to DropdownButtonFormField#182419
Make onChanged optional and add enabled to DropdownButtonFormField#182419Mairramer wants to merge 14 commits into
enabled to DropdownButtonFormField#182419Conversation
enabled to DropdownButtonFormField
|
Hi @Mairramer! Is this something you would like to move forward with? |
Yes, I’d like that, but only if the team genuinely believes it’s something worth pursuing and sees potential for me to move forward with it. |
justinmc
left a comment
There was a problem hiding this comment.
I'm on board with this approach. I agree that the situation described in #57953 is probably confusing a bunch of people.
My main question is whether this will cause breakages or not. Looks like customer tests are passing, but we should run the Google tests. If there are tons of breakages maybe we can rethink how we do this, but if not then I'm all for landing this.
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
# Conflicts: # packages/flutter/test/material/dropdown_test.dart
There was a problem hiding this comment.
Code Review
This pull request introduces an enabled property to DropdownButton and DropdownButtonFormField, allowing these widgets to be disabled independently of the onChanged callback. It includes data-driven fixes to migrate existing code that used onChanged: null for disabling the widgets. Review feedback suggests updating the documentation to clarify that an empty items list also disables the button and identifies a potential edge case in the migration logic where an explicit enabled: true alongside a null onChanged callback might not be handled correctly.
In this case, we would only have a small breaking change related to the conditional use of |
justinmc
left a comment
There was a problem hiding this comment.
Approving in order to kick off the Google tests.
There was a problem hiding this comment.
Thanks for including a dart fix
|
This will have to be postponed due to the code freeze, though. |
|
There are Google test failures, and they look real. |
|
@Mairramer I see Google tests that have failed because a previously disabled field is now enabled. Would it be possible to solve this problem in a way that doesn't break anything? |
Yes, I believe so, but I’ll need to adjust a few things in the implementation first to avoid introducing breaking changes and to keep the existing behavior intact. |
…for fallback behavior
|
Is it ready for another review? Or if the change is going to take some time, would you mind converting this PR to a draft for now? (from triage) |
Yes, it’s ready. The main thing now would be for someone with approval access to verify the Google testing results. |
|
I will approve to kick of Google testing to see if that is resolved, in the meantime, since this will be a breaking change, can you create a migration guide in flutter/website? We can land it when this is ready, but it will be good to have a look at the guidance and iterate there as well in the meantime. :) |
|
@Piinks Migration guide created: flutter/website#13430 |
justinmc
left a comment
There was a problem hiding this comment.
LGTM with nits 👍 . But due to the code freeze, we should not land this PR. It will have to be ported to flutter/packages after flutter/packages#11669 lands and we open up material_ui.
| /// If [enabled] is false, or the list of [DropdownButton.items] is null or empty, | ||
| /// then the dropdown button will be disabled. When disabled, the arrow is | ||
| /// displayed in grey and the button does not respond to input. |
There was a problem hiding this comment.
Should you also explain what happens if enabled is null? Which I think includes the original behavior of disabling when onChanged is null. Or maybe you are trying to encourage people to explicitly set enabled?
There was a problem hiding this comment.
In this case, it would be both. However, I’ll include the nullable enabled behavior as well.
Description of what this PR is changing or adding, and why: Adds a breaking change page for the new `enabled` property logic on `DropdownButton` and `DropdownButtonFormField`. This explains why the `onChanged` callback is no longer required and clarifies the fallback behavior for backward compatibility. It also guides developers on how to properly separate interactive state (`enabled`) from widget behavior (`onChanged`), highlighting what the `dart fix` covers and what requires manual migration (like conditional logic). Issues fixed by this PR (if any): N/A but related to flutter/flutter#57953 PRs or commits this PR depends on (if any): Depends on flutter/flutter#182419 ## Presubmit checklist - [x] If you are unwilling, or unable, to sign the CLA, even for a _tiny_, one-word PR, please file an issue instead of a PR. - [x] If this PR is not meant to land until a future stable release, mark it as draft with an explanation. - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style)—for example, it doesn't use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first-person pronouns). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Parker Lougheed <parlough@gmail.com>
Made
onChangedoptional and introduced theenabledparameter inDropdownButtonFormField, allowing proper disabled/read-only usage without dummy callbacks.Breaking change
Previously, setting
onChanged: nulldisabled the widget. This is no longer the case. The disabled state must now be controlled via theenabledproperty.Most simple usages are handled by the automated fix tool. Code that conditionally relied on
onChangedbeing null to disable the widget must be updated to useenabled.If not migrated, the widget will remain interactive but will not trigger a callback.
Closes #57953
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.