[Form] Add is valid to FormState by tjonganthony · Pull Request #48948 · flutter/flutter

tjonganthony · 2020-01-16T08:33:30Z

Description

Added isValid method that only checks whether every Field of a Form is valid.

Related Issues

Fixes #48874

Tests

I added the following tests:

Test whether isValid return the correct value when all field is valid
Test whether isValid return the correct value when at least 1 field is not valid

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

goderbauer · 2020-01-16T18:07:02Z

Thanks for the contribution. It looks like the analyzer is not happy. Can you fix that please?

tjonganthony · 2020-01-20T02:19:55Z

Thanks for the contribution. It looks like the analyzer is not happy. Can you fix that please?

Done.

goderbauer

As mentioned in the underling issue (#48874 (comment)) I am not sure if adding this is a good idea.

tjonganthony · 2020-01-24T02:12:05Z

Away for Chinese New Year now so I'm not with my workstation, will fix this PR after the holiday. Thanks for reviewing, Happy Holidays.

Piinks · 2020-01-27T19:31:33Z

Away for Chinese New Year now so I'm not with my workstation, will fix this PR after the holiday. Thanks for reviewing, Happy Holidays.

Happy Holidays to you as well, and thank you for the contribution! I will mark this as work in progress so it won't be pinged for updates. :)

goderbauer · 2020-01-30T22:11:57Z

I am still not convinced that this is the right way forward. It doesn't fit in with the existing form API. There's also the discussion in #48155, which should probably be resolved first before we add more API.

goderbauer · 2020-02-04T22:39:51Z

/cc @justinmc You recently looked into the Forms API as part of #48155. What's your take on this change (also note the discussion in the underlying bug: #48874).

justinmc

I'm going to side with supporting this change (with a few small changes below).

It seems to me that currently it's very awkward to check if a field is valid without causing the error on screen to update. Here is how I would do it, but please critique if there is a better way! https://dartpad.dartlang.org/34b602fa0a542a329dc5a5171ead9fc0

Besides requiring you to use a TextEditingController and a separate validation function, this isn't as robust as it could be. If I change the form field to use a different validation function instead of _validateEmail, I'll have to remember to update my code that checks if it's valid too. If you imagine this pattern on a very large form (@tjonganthony said he has 15 fields), then it's probably very messy.

I'm wary of adding complexity to a widget, but isValid is a very simple getter, and I don't see any problem with the pattern of passively checking if a field is valid. (Actually, it's validate() that I don't like, because it's an imperative method in a declarative framework.)

As I see it, I advocate for merging this and holding off on #48155. I'm open to discussion on all of this though!

This will allow FormState to check the validity of every field without updating the UI

Fix grammar. Co-Authored-By: Mohit Kanwal <creativepsyco@users.noreply.github.com>

justinmc · 2020-02-07T18:39:50Z

@tjonganthony I was talking this over with @goderbauer and @HansMuller and we can't think of a common use case for having isValidon the overall Form. Enabling/disabling a submit button but not showing any errors seems like something we shouldn't encourage. What do you think of only adding isValid to the fields and not the overall Form?

tjonganthony · 2020-02-10T03:57:38Z

@justinmc technically I don't really mind since we can always hold the key to each form field and get the state. However I wonder what's the reasoning behind not adding it to Form? Personally I don't think it would cause harm to have this state.

Maybe a bit of background for our project: This is the current behaviour of our native app before we migrate to flutter. Our UX wants user to at least edit the field before error is shown on that field, and keep the button disabled until the form is valid.
To reduce the friction of migration, we decided to keep UX behavioural change to minimum. Granted that our app was not yet fully compliant to GM2 behaviour, but it would be tough for us to do both migration and GM2 compliance. We do try to follow GM2 as much as possible in flutter while not having too much disruption in the migration as our highest priority right now is to complete the migration. I have discussed with the UX team and they do agree that this is definitely something to look at again in the future.

mehmetf · 2020-02-10T17:20:18Z

Enabling/disabling a submit button but not showing any errors seems like something we shouldn't encourage.

I agree with this if you consider the use cases from user's point of view:

If I have not changed the value of a field yet, don't show any errors to me. I likely know what I need to put in but have not got a chance to do it yet.
- Don't show errors if I have not interacted with the field.
- Don't show errors if I tapped on a field to focus but then blurred it without changing anything.
If I try to submit a form, show errors to me even if I have not interacted with the field. This is an example of implicit interaction. The user believes the field is correct by giving you a signal with "submit".

If you take away the last use case so that user cannot submit the form, the user no longer knows why the form cannot be submitted. The developer also no longer knows what the intent of the user is. Are they trying to submit? or are they still trying to fill out the form?

That's where the harm is. If you put isValid() on the entire form without triggering errors, you encourage people to silently validate an entire form which should theoretically only be done upon user action.

As you said, you can still do this but Flutter, as a framework, should not encourage it.

goderbauer · 2020-02-10T17:23:25Z

As you said, you can still do this but Flutter, as a framework, should not encourage it.

+1 to this.

I am ok with having a validator on the form field, but putting it on the entire From seems off.

…is-valid-to-form-state

tjonganthony · 2020-02-12T02:43:13Z

Alright, that all sounds fair. As I mentioned earlier, I don't have any issue with this behaviour being discouraged. Changed as per suggested :)

goderbauer

/cc @justinmc for detailed review.

justinmc

LGTM, just a few optional nits. Thanks for all the discussion on these issues, I think we're really making our forms better 👍

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 16, 2020

googlebot added the cla: yes label Jan 16, 2020

tjonganthony changed the title ~~Feature/add is valid to form state~~ [Form] Add is valid to FormState Jan 16, 2020

tjonganthony requested review from HansMuller and mehmetf January 16, 2020 08:34

creativepsyco approved these changes Jan 16, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/test/widgets/form_test.dart Outdated

SpajicM mentioned this pull request Jan 20, 2020

Add FormField validation states (touched, saved, etc) with ErrorStateMatcher #48155

Closed

10 tasks

goderbauer reviewed Jan 22, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/test/widgets/form_test.dart Outdated

Piinks added work in progress; do not review c: new feature Nothing broken; request for a new capability labels Jan 27, 2020

tjonganthony requested a review from goderbauer January 30, 2020 01:16

Piinks removed the work in progress; do not review label Jan 30, 2020

justinmc reviewed Feb 5, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/test/widgets/form_test.dart Outdated

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

tjonganthony and others added 9 commits February 7, 2020 14:49

[Form] Add isValid method to FormState

afe1bc6

This will allow FormState to check the validity of every field without updating the UI

Undo formatting

f712791

Apply suggestions from code review

149ac6a

Fix grammar. Co-Authored-By: Mohit Kanwal <creativepsyco@users.noreply.github.com>

Fix analizer error

486acc6

improve documentation and test

88c3c16

fix analyzer

070823b

fix more analyzer warning

44c2ff5

improve documentation and improve minor performance based on review

877f6ee

fix trailing space

935e9bf

tjonganthony force-pushed the feature/add-is-valid-to-form-state branch from 3c914da to 935e9bf Compare February 7, 2020 06:50

tjonganthony added 2 commits February 12, 2020 10:35

remove isValid from FormState as per discussion in review

ba215e5

Merge branch 'master' of github.com:flutter/flutter into feature/add-…

7fc5a1e

…is-valid-to-form-state

goderbauer reviewed Feb 12, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/form.dart

Add an empty line for dartdoc.

496f870

justinmc approved these changes Feb 18, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

Comment thread packages/flutter/lib/src/widgets/form.dart Outdated

tjonganthony removed request for HansMuller and mehmetf February 18, 2020 21:27

More update on dartdoc to improve clarity.

7335084

justinmc added the waiting for tree to go green label Feb 18, 2020

fluttergithubbot merged commit e7c9005 into flutter:master Feb 19, 2020

tjonganthony deleted the feature/add-is-valid-to-form-state branch February 20, 2020 15:33

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Uh oh!

Conversation

tjonganthony commented Jan 16, 2020 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Jan 16, 2020

Uh oh!

tjonganthony commented Jan 20, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tjonganthony commented Jan 24, 2020

Uh oh!

Piinks commented Jan 27, 2020

Uh oh!

goderbauer commented Jan 30, 2020

Uh oh!

goderbauer commented Feb 4, 2020

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc commented Feb 7, 2020

Uh oh!

tjonganthony commented Feb 10, 2020

Uh oh!

mehmetf commented Feb 10, 2020

Uh oh!

goderbauer commented Feb 10, 2020

Uh oh!

tjonganthony commented Feb 12, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tjonganthony commented Jan 16, 2020 •

edited by Piinks

Loading