Sitelet https://github.com/flutter/flutter/pull/2850
Skip to content

Added a Form widget to manage multiple Input widgets. - #2850

Merged
mpcomplete merged 1 commit into
flutter:masterfrom
mpcomplete:form
Mar 28, 2016
Merged

mpcomplete merged 1 commit into
flutter:masterfrom
mpcomplete:form

Conversation

@mpcomplete

Copy link
Copy Markdown
Contributor

No description provided.

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.

always put a comment in the block of a setState saying what state it is you are changing.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

OK, I uploaded a new iteration. Main changes:

  • No more Maps. InputState holds onto the _FormFieldData (previously _FormFieldState) rather than shoving it in a Map on the _FormState.
  • No more form IDs. I replaced them with generic setters, which lets the user can store the Form data anyway they wish.

I will move Form and all related classes to a separate file next (except _FormFieldData, which is specific to Input). I kept it as-is so you could see the changes with the previous version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I name this back to _FormFieldState? It doesn't derive from State, but it is used only as a member var of InputState. And it does represent the 'state' of the form field...

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.

if you don't like _FormFieldData, maybe _FormFieldValue? By convention, if the class ends with State that means it inherits from State<T>.

@HansMuller

Copy link
Copy Markdown
Contributor

LGTM

Hopefully before too long we can begin using tear-offs for the setter functions.

@Hixie

Hixie commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

This needs tests.

If I understood the original purpose of Form correctly, it was to make sure that you didn't need to store the state of form fields in the object whose build function creates the Inputs, even if the build function changed its structure each frame. I don't think this feature exists any more in the patch as it stands today.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

The original purpose of Form was so client code (e.g. text_field_demo.dart) didn't need to store a list of InputValues. This way, it stores the form data in a way that is natural to the client (struct PersonData). The InputValue book-keeping is handled by the Form class.

@Hixie

Hixie commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

But the reason the client code kept the InputValues was so that if the build changed structure, the selection wouldn't be blown away. Isn't that lost with this change?

@mpcomplete

Copy link
Copy Markdown
Contributor Author

No, without a Form, the client code needs to keep InputValues no matter what. That's the only way the input value will change at all (Input doesn't remember what was typed).

I don't think the form data or selection will be blown away - as long as the Inputs have keys. That will ensure they have the same InputState, right? But even if not, that was not my original goal.

@Hixie

Hixie commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

Fair enough. I'll file a bug to worry about that problem later.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

I've added tests in form_test.dart.

@Hixie

Hixie commented Mar 28, 2016

Copy link
Copy Markdown
Contributor

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants