Skip to content

Form fields onChange callback should be called on reset - #134295

Merged
auto-submit[bot] merged 2 commits into
flutter:masterfrom
NevercodeHQ:formfield_onchange_should_be_called_after_reset
Sep 21, 2023
Merged

Form fields onChange callback should be called on reset#134295
auto-submit[bot] merged 2 commits into
flutter:masterfrom
NevercodeHQ:formfield_onchange_should_be_called_after_reset

Conversation

@bleroux

@blerouxbleroux commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

Description

This PR fixes form fields in order to call the onChange callback when the form is reset.

This change is based on the work done in #123108.

I considered adding the onChange callback to the FormField superclass but it would break existing code because two of the three subclasses defines the onChange callback with ValueChanged<String>? type and the third one defines it with ValueChanged<String?>?.

Related Issue

Fixes#123009.

Tests

Adds 3 tests.

@github-actionsgithub-actionsBot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages p: cupertino_ui cupertino_ui package in flutter/packages labels Sep 8, 2023
@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch 5 times, most recently from 90eea5c to ba88062CompareSeptember 12, 2023 12:38
@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch from ba88062 to 2af641fCompareSeptember 15, 2023 07:10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we still call this if the value did not change?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same for other FormFields.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I chose not to add such a check in order to be consistent with FormState.reset which (indirectly) call the Form.onChanged callback without checking if field values have changed.
I also checked that DropdownButton.onChanged is called when the same entry is selected.

Maybe we should consider this is another issue/PR? I don't know if this was done on purpose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be made a doc template and reused.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Probably, I upated the PR accordingly.

Comment threadpackages/flutter/test/material/text_form_field_test.dart Outdated
Comment threadpackages/flutter/test/material/text_form_field_test.dart Outdated

@Renzo-OlivaresRenzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @bleroux. Just had a few comments.

@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch from 2af641f to 28efbeeCompareSeptember 20, 2023 06:31
@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch from 28efbee to fc3af56CompareSeptember 20, 2023 09:19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: TestFormField -> TextFormField. I think

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for pointing this out!

@Renzo-OlivaresRenzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM w/ small nit. Thanks for fixing this @bleroux!

@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch 2 times, most recently from 914a2fa to 57c90ccCompareSeptember 20, 2023 18:50
@bleroux
blerouxforce-pushed the formfield_onchange_should_be_called_after_reset branch from 57c90cc to 0ec68a8CompareSeptember 20, 2023 20:15
@blerouxbleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 21, 2023
@auto-submit
auto-submitBot merged commit 07cc3f7 into flutter:masterSep 21, 2023
@bleroux
bleroux deleted the formfield_onchange_should_be_called_after_reset branch September 21, 2023 05:54
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
## Description
This PR fixes form fields in order to call the `onChange` callback when the form is reset.
This change is based on the work done in flutter#123108.
I considered adding the `onChange` callback to the `FormField` superclass but it would break existing code because two of the three subclasses defines the `onChange` callback with `ValueChanged<String>?` type and the third one defines it with `ValueChanged<String?>?`. ## Related Issue
Fixesflutter#123009.
## Tests
Adds 3 tests.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text inputEntering text in a text field or keyboard related problemsautosubmitMerge PR when tree becomes green via auto submit Appframeworkflutter/packages/flutter repository. See also f: labels.p: cupertino_uicupertino_ui package in flutter/packagesp: material_uimaterial_ui package in flutter/packages

Projects

None yet

2 participants

@bleroux@Renzo-Olivares