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

Form fields onChange callback should be called on reset #134295

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Sep 8, 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 #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-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Sep 8, 2023
@bleroux bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch 5 times, most recently from 90eea5c to ba88062 Compare September 12, 2023 12:38
@bleroux bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch from ba88062 to 2af641f Compare September 15, 2023 07:10
_effectiveController!.text = widget.initialValue!;
});
}
_cupertinoTextFormFieldRow.onChanged?.call(_effectiveController!.text);
Copy link
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
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
Contributor Author

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.

@@ -272,6 +270,10 @@ class TextFormField extends FormField<String> {
/// initialize its [TextEditingController.text] with [initialValue].
final TextEditingController? controller;

/// Called when the user initiates a change to the TextField's
Copy link
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
Contributor Author

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.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

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 bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch from 2af641f to 28efbee Compare September 20, 2023 06:31
@bleroux bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch from 28efbee to fc3af56 Compare September 20, 2023 09:19
@@ -272,6 +270,12 @@ class TextFormField extends FormField<String> {
/// initialize its [TextEditingController.text] with [initialValue].
final TextEditingController? controller;

/// {@template flutter.material.TestFormField.onChanged}
Copy link
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
Contributor Author

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!

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

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 bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch 2 times, most recently from 914a2fa to 57c90cc Compare September 20, 2023 18:50
@bleroux bleroux force-pushed the formfield_onchange_should_be_called_after_reset branch from 57c90cc to 0ec68a8 Compare September 20, 2023 20:15
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 21, 2023
@auto-submit auto-submit bot merged commit 07cc3f7 into flutter:master Sep 21, 2023
67 checks passed
@bleroux bleroux deleted the formfield_onchange_should_be_called_after_reset branch September 21, 2023 05:54
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

Fixes flutter#123009.

## Tests

Adds 3 tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
2 participants