-
Notifications
You must be signed in to change notification settings - Fork 29.5k
Add DropdownMenuFormField #163721
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 DropdownMenuFormField #163721
Conversation
66b728c to
66b1d60
Compare
|
For the moment only the selected value is considered for the form field, the content of the text field is not considered (no validation when changed), this is because currently the text field is meant to be use to filter and search entries. @justinmc Do you think we should include the text field content also? It probably makes sense when the DropdownMenu is used as a combobox. |
justinmc
left a comment
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.
Sorry for the slow review! Some questions but otherwise this looks good. I think it's valuable to create this FormField for users, thanks for doing it.
packages/flutter/lib/src/material/dropdown_menu_form_field.dart
Outdated
Show resolved
Hide resolved
| super.reset(); | ||
| _dropdownMenuFormField.onSelected?.call(value); | ||
| } | ||
| } |
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.
Would it be useful to also override restoreState, like TextFormField does?
| void restoreState(RestorationBucket? oldBucket, bool initialRestore) { |
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 looked at this, the main problem is that Restoration API works for basic types (String, int, etc) but not for any subclass of Object (until serialization code is provided). And the selected value for a DropdownMenu could be of any type.
I will try to use the restoration API just for the inner text field (more precisely for its text controller) and in restoreState when a value is restored it might possible to check if it matches one of the dropdown menu entries.
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.
Ah true. Let me know how it goes, it would be nice to have if possible. Also add a test for it if you get it working.
packages/flutter/lib/src/material/dropdown_menu_form_field.dart
Outdated
Show resolved
Hide resolved
packages/flutter/test/material/dropdown_menu_form_field_test.dart
Outdated
Show resolved
Hide resolved
I'd appreciate this, as the form can be submitted while the text field still has text in it and I'm not sure if a value is even selected (or which value is selected) when submitting |
66b1d60 to
74921f5
Compare
74921f5 to
97a94cb
Compare
a9c6889 to
ba074b1
Compare
justinmc
left a comment
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.
Sounds like there are two comments you're still looking into, but LGTM either way you go with them. Thanks for looking into those.
| super.reset(); | ||
| _dropdownMenuFormField.onSelected?.call(value); | ||
| } | ||
| } |
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.
Ah true. Let me know how it goes, it would be nice to have if possible. Also add a test for it if you get it working.
packages/flutter/lib/src/material/dropdown_menu_form_field.dart
Outdated
Show resolved
Hide resolved
1818c5d to
b1276b7
Compare
|
Once #166684 will be merged, I will update this PR to add support for restoration API. |
## Description This PR introduces `DropdownMenu.restorationId`. This value is passed to the inner `TextField.restorationId` which is required to activate the TextField restoration capability. ## Related Issue Required for #163721 ## Tests Adds 1 test.
justinmc
left a comment
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 with the restorationId and test, thanks for adding that!
|
Thanks for the approval. #166684 was merged, now I have to update this PR to make use of it but also have to leverage |
## Description This PR introduces `DropdownMenu.restorationId`. This value is passed to the inner `TextField.restorationId` which is required to activate the TextField restoration capability. ## Related Issue Required for flutter#163721 ## Tests Adds 1 test.
## Description This PR introduces DropdownMenuFormField. ## Related Issue Fixes [Create DropdownMenuFormField](flutter#141941) Fixes [Add validator to DropdownMenu](flutter#152131) ## Tests Adds 41 tests.
|
@bleroux, which channel currently has this Feature ? |
|
For the moment, it is only on master. |
## Description This PR introduces `DropdownMenu.restorationId`. This value is passed to the inner `TextField.restorationId` which is required to activate the TextField restoration capability. ## Related Issue Required for flutter#163721 ## Tests Adds 1 test.
Is this being tracked somewhere? Currently if you make a selection then type random text there doesn't seem to be an easy way get feedback that the previous selection is no longer valid (except indirectly using the I believe there should be an |
@nebkat I don't think an issue was created to track this. It would be helpful if you can create one and describe the use cases. It would a place to collect feedback and discuss possible solutions. |
Description
This PR introduces DropdownMenuFormField.
Related Issue
Fixes Create DropdownMenuFormField
Fixes Add validator to DropdownMenu
Tests
Adds 41 tests.