-
Notifications
You must be signed in to change notification settings - Fork 28.6k
Introduce new Form validation method #135578
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
/// to the invalid field(s) using their widget key(s). | ||
/// | ||
/// The form will rebuild to report the results. | ||
Map<Key, bool> validateGranually() { |
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.
Maybe just return a collection of fields that hasError
?
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.
Also the method name doesn't seem very self-explanatory when I first read it.
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.
Maybe just return a collection of fields that hasError?
I'm avoiding returning the field itself because FormFieldState
doesn't look like a class that's meant to be exposed to a public API. Otherwise, why _fields
in FormState
has no public getter in the first place? imo FormFieldState
is a public class only to be used as a param to the builder when extending FormField
.
Please lmk if you disagree. In that case, we have two better approaches to achieving #135363 :
- We can add a public getter for
_fields
and revert all the changes, as the user would be able to call isValid on the fields individually. - We return a collection of all the fields with error as you're suggesting, thus resolving your other comment below regarding only including keyed fields.
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.
Also the method name doesn't seem very self-explanatory when I first read it.
I totally agree but couldn't come up with a better name. Any suggestions?
Maybe:
- validateAndGetResults
- validateWithResults
- getInvalidFields (if we decided to return only the fields with error as you're suggesting)
- ..?
Optimally, I'd call this methodvalidate
and call the old methodvalidateAll
, but it's a breaking change so I don't think it's an option.
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.
FormFieldState
is meant to be public I think (it has public methods, e.g., https://master-api.flutter.dev/flutter/widgets/FormFieldState/save.html). Also Key
s aren't necessarily unique.
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.
@LongCatIsLooong I think it's the responsibility of the user to use unique keys for widgets that share a parent, otherwise they will get a "Duplicate Key found" error
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.
Yeah but FormField
s don't have to share a parent. The Form
just has to be the ancestor of all the fields.
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.
@LongCatIsLooong They will still get "Duplicate Key found" error in that case too. Using unique keys is the user's responsibility, even before this pr.
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.
While GlobalKey
s must be unique in the widget tree, Key
s don't have to be unique.
bool hasError = false; | ||
String errorMessage = ''; | ||
for (final FormFieldState<dynamic> field in _fields) { | ||
hasError = !field.validate() || hasError; | ||
errorMessage += field.errorText ?? ''; | ||
final Key? key = field.widget.key; | ||
if (key != null) { | ||
fieldsValidationStatus[key] = field.validate(); |
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.
validate()
is called twice in this method.
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.
Also I think we probably shouldn't change _validate
's implementation like this. validate
only returns a bool and the Map
populated ends up getting thrown away.
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.
validate() is called twice in this method.
You're right 👍 I'll fix.
the Map populated ends up getting thrown away.
The map is used in the new method validateGranulay
.
validate
and validateGranulay
should share the same logic except for the returned value. That's why I added the wrapper class _FormValidationStatus
and avoided duplicating code, for example, the code for announcing the error.
I could extract the shared code to a private method and call it in _validate
and validateGranualy
, but any future changes that involve performing some side effects on validation will also need to be written twice.
It's just an opinion, please lmk if you disagree I'll make the changes by extraction.
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.
@SharbelOkzan Would it work if you passed a optional map to the _validate
method?
So it would be
Map<Key, bool> validateGranually() {
final Map<Key, bool> results = <Key, bool>{};
_hasInteractedByUser = true;
_forceRebuild();
_validate(results);
return results;
}
bool validate() {
_hasInteractedByUser = true;
_forceRebuild();
return _validate();
}
bool _validate([Map<Key, bool>? optionalResults]) { // This can also be _validate({}) for named params.
bool hasError = false;
String errorMessage = '';
for (final FormFieldState<dynamic> field in _fields) {
hasError = !field.validate() || hasError;
errorMessage += field.errorText ?? '';
final Key? key = field.widget.key;
if (optionalResults != null && key != null) {
optionalResults[key] = field.validate();
}
}
if (errorMessage.isNotEmpty) {
final TextDirection directionality = Directionality.of(context);
if (defaultTargetPlatform == TargetPlatform.iOS) {
unawaited(Future<void>(() async {
await Future<void>.delayed(_kIOSAnnouncementDelayDuration);
SemanticsService.announce(errorMessage, directionality, assertiveness: Assertiveness.assertive);
}));
} else {
SemanticsService.announce(errorMessage, directionality, assertiveness: Assertiveness.assertive);
}
}
return !hasError;
}
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.
@Renzo-Olivares Yes, but I thought pure functions are preferred. Anw I updated pr with your suggestion 👍
(triage) @SharbelOkzan @LongCatIsLooong what's the status of this PR? |
@@ -293,19 +293,45 @@ class FormState extends State<Form> { | |||
/// returns true if there are no errors. | |||
/// | |||
/// The form will rebuild to report the results. | |||
/// See also: [validateGranually] |
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.
nit: this should be formatted like other See also:
in the framework.
For example from SelectableRegion
:
/// See also:
/// * [SelectableRegion], which provides an overview of the selection system.
@@ -611,3 +637,19 @@ enum AutovalidateMode { | |||
/// interaction. | |||
onUserInteraction, | |||
} | |||
|
|||
// Used to encapsultae `Form`'s validation state. |
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.
nit: encapsultae
-> encapsulate
.
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.
nit: Form
's -> Form
s I think.
@@ -611,3 +637,19 @@ enum AutovalidateMode { | |||
/// interaction. | |||
onUserInteraction, | |||
} | |||
|
|||
// Used to encapsultae `Form`'s validation state. | |||
// Having all vlaues of `fieldsValidationStatus` as true is not a guarantee |
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.
nit: vlaues
-> values
.
// Having all vlaues of `fieldsValidationStatus` as true is not a guarantee | ||
// of a valid form. Fields with no keys are skipped in this | ||
// Map. | ||
// a Form with no validation erros is only represented by setting `isValid` to true. |
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.
nit: Capitalize a
in this sentence.
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.
nit: erros
-> errors
.
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.
Hi @SharbelOkzan, thank you for the contribution! This change sounds reasonable to me, I have made a few comments regarding some implementation details. Let me know what you think.
Thanks @Renzo-Olivares. Updated, sorry for the many typos |
@@ -293,18 +293,46 @@ class FormState extends State<Form> { | |||
/// returns true if there are no errors. | |||
/// | |||
/// The form will rebuild to report the results. | |||
/// See also: |
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.
nit: should have an empty new line before this line.
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.
Hi @Renzo-Olivares , done. Anything else I should fix?
See this comment: #135578 (comment). |
@LongCatIsLooong Sorry for the delay, pr updated. |
@@ -291,18 +291,44 @@ class FormState extends State<Form> { | |||
/// returns true if there are no errors. | |||
/// | |||
/// The form will rebuild to report the results. | |||
/// | |||
/// See also: | |||
/// * [validateGranularly], which returns a [Map] describing the validation |
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.
Looks like it's no longer a Map
?
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.
"which also validates descendant FormFields, but instead returns fields with errors" or something similar?
bool validate() { | ||
_hasInteractedByUser = true; | ||
_forceRebuild(); | ||
return _validate(); | ||
} | ||
|
||
bool _validate() { | ||
|
||
/// Validates every [FormField]s that is a descendant of this [Form], and |
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.
FormFields -> FormField
bool _validate() { | ||
|
||
/// Validates every [FormField]s that is a descendant of this [Form], and | ||
/// returns a [List] of [FormFieldState] of the invalid field(s) only, if any. |
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.
Does the order matter? Should we return an unordered collection instead?
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.
The order could matter. It doesn't have to be predictable but has to be consistent. i.e. validateGranularly().first
should return the same field as long as it's still invalid.
I replaced List with Set, but I don't think we can replace it with something not ordered like HashSet
. Let me know if you have other collections in mind.
The rest of the comments are addressed too.
/// Validates every [FormField]s that is a descendant of this [Form], and | ||
/// returns a [List] of [FormFieldState] of the invalid field(s) only, if any. | ||
/// | ||
/// To get the absolute validation value of the Form, Consider using [validate] |
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.
Maybe turn this into a "Seealso".
/// | ||
/// To get the absolute validation value of the Form, Consider using [validate] | ||
/// | ||
/// Common usage of this method is when you need draw the user's attention |
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.
Maybe: This method can be useful when ... / can be used to highlight fields with errors.
/// to the invalid field(s) using their widget key(s). | ||
/// | ||
/// The form will rebuild to report the results. | ||
List<FormFieldState<dynamic>> validateGranularly() { |
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.
nit: dynamic
-> Object?
/// | ||
/// The form will rebuild to report the results. | ||
List<FormFieldState<dynamic>> validateGranularly() { | ||
final List<FormFieldState<dynamic>> invalidFields = List<FormFieldState<dynamic>>.empty(growable: true); |
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.
use List literal to instantiate this instead? (also why List?)
/// returns a [Set] of [FormFieldState] of the invalid field(s) only, if any. | ||
/// | ||
/// This method can be useful to highlight field(s) with errors, | ||
/// possibly using their widget key(s). |
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.
Looks like the documentation here needs updating?
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.
It still holds IMO. The widget keys are still most likely what the user will use to highlight fields with errors on the screen.
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.
If you have the states you don't need keys no?
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.
^^ maybe this can just be This method can be useful to highlight field(s) with errors.
/// | ||
/// See also: | ||
/// * [validate], which also validates descendant [FormField]s, | ||
/// and return true if there are no errors |
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.
missing "." at the end.
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.
nit: "but returns a boolean value indicating whether the [Form] has invalid user inputs" or something like that?
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.
That's actually the docs for validate
, it was there before this pr, I just copied it to the "see also".
/// | ||
/// See also: | ||
/// * [validateGranularly], which also validates descendant [FormField]s, | ||
/// but instead returns a [Set] of fields with errors |
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.
missing "." at the end of the sentence.
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 modulo nits.
/// returns a [Set] of [FormFieldState] of the invalid field(s) only, if any. | ||
/// | ||
/// This method can be useful to highlight field(s) with errors, | ||
/// possibly using their widget key(s). |
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.
If you have the states you don't need keys no?
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 pending the comment regarding keys.
Thanks @Renzo-Olivares. Updated. |
flutter/flutter@126302d...b840a60 2024-01-10 [email protected] Roll Flutter Engine from 3269fd84460d to b361a60ae224 (1 revision) (flutter/flutter#141271) 2024-01-10 [email protected] Roll Flutter Engine from a5d446da5495 to 3269fd84460d (1 revision) (flutter/flutter#141264) 2024-01-10 [email protected] Roll Flutter Engine from 3ccf66bed335 to a5d446da5495 (2 revisions) (flutter/flutter#141252) 2024-01-10 [email protected] Roll Flutter Engine from e57e418c02ae to 3ccf66bed335 (1 revision) (flutter/flutter#141241) 2024-01-10 [email protected] Roll Flutter Engine from 7e6f3d847e01 to e57e418c02ae (1 revision) (flutter/flutter#141240) 2024-01-10 [email protected] Roll Flutter Engine from 1cf2e0a603c7 to 7e6f3d847e01 (1 revision) (flutter/flutter#141237) 2024-01-10 [email protected] Roll Flutter Engine from 32bbf8be8d2c to 1cf2e0a603c7 (1 revision) (flutter/flutter#141232) 2024-01-10 [email protected] Roll Flutter Engine from 5b9d2132b7cd to 32bbf8be8d2c (1 revision) (flutter/flutter#141229) 2024-01-10 [email protected] Roll Flutter Engine from 941f268fc8bb to 5b9d2132b7cd (1 revision) (flutter/flutter#141228) 2024-01-10 [email protected] Roll Flutter Engine from 542fea9edae4 to 941f268fc8bb (3 revisions) (flutter/flutter#141224) 2024-01-10 [email protected] `NestedScrollView`'s outer scrollable jumping with `BouncingScrollPhysics` due to `double` precision errors (flutter/flutter#138319) 2024-01-10 [email protected] Roll Flutter Engine from 8c1501f3956d to 542fea9edae4 (1 revision) (flutter/flutter#141217) 2024-01-10 [email protected] Fix or except leaks. (flutter/flutter#141081) 2024-01-09 [email protected] Roll Flutter Engine from 693af0c699c5 to 8c1501f3956d (1 revision) (flutter/flutter#141215) 2024-01-09 [email protected] TextStyle: In copyWith, stop ignoring debugLabel when receiver has none (flutter/flutter#141141) 2024-01-09 [email protected] Roll Flutter Engine from a35e3b026e1d to 693af0c699c5 (5 revisions) (flutter/flutter#141209) 2024-01-09 [email protected] Replace deprecated `exists` in podhelper.rb (flutter/flutter#141169) 2024-01-09 [email protected] Correctly handle null case in ProcessText.queryTextActions (flutter/flutter#141205) 2024-01-09 [email protected] Add environment variable to leak tracking bots. (flutter/flutter#141137) 2024-01-09 [email protected] Reapply "Dynamic view sizing" (#140165) (flutter/flutter#140918) 2024-01-09 [email protected] Introduce new Form validation method (flutter/flutter#135578) 2024-01-09 [email protected] Update `RouteObserver` example and fix an error thrown (flutter/flutter#141166) 2024-01-09 [email protected] Upgrade leak_tracker. (flutter/flutter#141153) 2024-01-09 [email protected] [ci.yaml] Do not run packaging test on presubmit (flutter/flutter#141192) 2024-01-09 [email protected] Roll Flutter Engine from 036b39fa47fa to a35e3b026e1d (6 revisions) (flutter/flutter#141191) 2024-01-09 [email protected] Run tests on iOS 16 or iOS 17 (flutter/flutter#141178) 2024-01-09 [email protected] Remove conditions that depend on order. (flutter/flutter#141183) 2024-01-09 [email protected] Roll Flutter Engine from b3c8597df0e2 to 036b39fa47fa (1 revision) (flutter/flutter#141179) 2024-01-09 [email protected] resolved the issue of indeterminate CircularProgressIndicator.adaptive on Darwin (flutter/flutter#140947) 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 [email protected],[email protected],[email protected] 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
flutter/flutter@126302d...b840a60 2024-01-10 [email protected] Roll Flutter Engine from 3269fd84460d to b361a60ae224 (1 revision) (flutter/flutter#141271) 2024-01-10 [email protected] Roll Flutter Engine from a5d446da5495 to 3269fd84460d (1 revision) (flutter/flutter#141264) 2024-01-10 [email protected] Roll Flutter Engine from 3ccf66bed335 to a5d446da5495 (2 revisions) (flutter/flutter#141252) 2024-01-10 [email protected] Roll Flutter Engine from e57e418c02ae to 3ccf66bed335 (1 revision) (flutter/flutter#141241) 2024-01-10 [email protected] Roll Flutter Engine from 7e6f3d847e01 to e57e418c02ae (1 revision) (flutter/flutter#141240) 2024-01-10 [email protected] Roll Flutter Engine from 1cf2e0a603c7 to 7e6f3d847e01 (1 revision) (flutter/flutter#141237) 2024-01-10 [email protected] Roll Flutter Engine from 32bbf8be8d2c to 1cf2e0a603c7 (1 revision) (flutter/flutter#141232) 2024-01-10 [email protected] Roll Flutter Engine from 5b9d2132b7cd to 32bbf8be8d2c (1 revision) (flutter/flutter#141229) 2024-01-10 [email protected] Roll Flutter Engine from 941f268fc8bb to 5b9d2132b7cd (1 revision) (flutter/flutter#141228) 2024-01-10 [email protected] Roll Flutter Engine from 542fea9edae4 to 941f268fc8bb (3 revisions) (flutter/flutter#141224) 2024-01-10 [email protected] `NestedScrollView`'s outer scrollable jumping with `BouncingScrollPhysics` due to `double` precision errors (flutter/flutter#138319) 2024-01-10 [email protected] Roll Flutter Engine from 8c1501f3956d to 542fea9edae4 (1 revision) (flutter/flutter#141217) 2024-01-10 [email protected] Fix or except leaks. (flutter/flutter#141081) 2024-01-09 [email protected] Roll Flutter Engine from 693af0c699c5 to 8c1501f3956d (1 revision) (flutter/flutter#141215) 2024-01-09 [email protected] TextStyle: In copyWith, stop ignoring debugLabel when receiver has none (flutter/flutter#141141) 2024-01-09 [email protected] Roll Flutter Engine from a35e3b026e1d to 693af0c699c5 (5 revisions) (flutter/flutter#141209) 2024-01-09 [email protected] Replace deprecated `exists` in podhelper.rb (flutter/flutter#141169) 2024-01-09 [email protected] Correctly handle null case in ProcessText.queryTextActions (flutter/flutter#141205) 2024-01-09 [email protected] Add environment variable to leak tracking bots. (flutter/flutter#141137) 2024-01-09 [email protected] Reapply "Dynamic view sizing" (#140165) (flutter/flutter#140918) 2024-01-09 [email protected] Introduce new Form validation method (flutter/flutter#135578) 2024-01-09 [email protected] Update `RouteObserver` example and fix an error thrown (flutter/flutter#141166) 2024-01-09 [email protected] Upgrade leak_tracker. (flutter/flutter#141153) 2024-01-09 [email protected] [ci.yaml] Do not run packaging test on presubmit (flutter/flutter#141192) 2024-01-09 [email protected] Roll Flutter Engine from 036b39fa47fa to a35e3b026e1d (6 revisions) (flutter/flutter#141191) 2024-01-09 [email protected] Run tests on iOS 16 or iOS 17 (flutter/flutter#141178) 2024-01-09 [email protected] Remove conditions that depend on order. (flutter/flutter#141183) 2024-01-09 [email protected] Roll Flutter Engine from b3c8597df0e2 to 036b39fa47fa (1 revision) (flutter/flutter#141179) 2024-01-09 [email protected] resolved the issue of indeterminate CircularProgressIndicator.adaptive on Darwin (flutter/flutter#140947) 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 [email protected],[email protected],[email protected] 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
Introduced
validateGranually
which, apart from announcing the errors to the UI, returns aMap<Key, bool>
providing more granular validation details: The results of callingvalidate
on eachFormField
and their corresponding widget keys.Pre-launch Checklist
///
).