Skip to content

Rationalize text input widgets - #9119

Merged
abarth merged 1 commit into
flutter:masterfrom
abarth:text_controller3
Apr 2, 2017
Merged

abarth merged 1 commit into
flutter:masterfrom
abarth:text_controller3

Conversation

@abarth

@abarth abarth commented Mar 31, 2017 •

Copy link
Copy Markdown
Contributor

After this patch, there are three major text input widgets:

  • EditableText. This widget is a low-level editing control that
    interacts with the IME and displays a blinking cursor.

  • TextField. This widget is a Material Design text field, with all the
    bells and whistles. It is highly configurable and can be reduced down
    to a fairly simple control by setting its decoration property to
    null.

  • TextFormField. This widget is a FormField that wraps a TextField.

This patch also replaces the InputValue data model for these widgets
with a Listenable TextEditingController, which is much more flexible.

Fixes #7031

@abarth

abarth commented Mar 31, 2017 •

Copy link
Copy Markdown
Contributor Author

This patch is stacked on #9074, so you probably want to just look at the second commit independently.

Also, I need to add/update a bunch of docs, but this patch should otherwise be ready for review.

/cc @Hixie

@abarth
abarth force-pushed the text_controller3 branch from e064a3c to 3530239 Compare March 31, 2017 09:45

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.

According to the Material mocks, the * should be red when the field doesn't have a value (even when the label is another colour). We didn't support this before, but it's something worth bearing in mind. Maybe a reason to take a Text here or something.

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 we factored this out into the InputDecoration class itself, then you could pass in entirely novel decorations, much as we do with DecoratedBox.

Actually that kind of seems like a primitive we should have. build(BuildContext context, Widget child). AnimatedBuilder does something like that, though it passes in a Listenable as well IIRC.

@Hixie Hixie Mar 31, 2017 •

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.

Actually AnimatedBuilder's builder does have exactly that signature. The listenable is passed in out of band.

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.

Maybe AnimatedBuilder is exactly what we should use here, with InputDecoration just being a class that extends Listenable and exposes a method with that signature, and you pass in both the Decoration and the method, or something...

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.

It's not quite so clean because there's some interaction between the InputDecoration data and the other parameters to the widget. For example, the baseStyle and the isFocused bit.

Also, you don't gain a ton by doing that because you can always pass null to TextField to remove the decoration and just wrap it yourself with whatever you want.

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.

something's off with this code.

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.

Yeah, that code was outdated. I've removed it since the point it's going after doesn't make sense anymore.

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.

does this actually work?

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.

It does something, but not much.

@Hixie

Hixie commented Mar 31, 2017

Copy link
Copy Markdown
Contributor

This looks great. LGTM.

If you add or edit docs in this patch and want that reviewed too, add them as a separate commit and I'll be happy to review those too.

@abarth

abarth commented Mar 31, 2017

Copy link
Copy Markdown
Contributor Author

I updated the docs, but I squashed the commit before seeing your note. :(

After this patch, there are three major text input widgets:

 * EditableText. This widget is a low-level editing control that
   interacts with the IME and displays a blinking cursor.

 * TextField. This widget is a Material Design text field, with all the
   bells and whistles. It is highly configurable and can be reduced down
   to a fairly simple control by setting its `decoration` property to
   null.

 * TextFormField. This widget is a FormField that wraps a TextField.

This patch also replaces the InputValue data model for these widgets
with a Listenable TextEditingController, which is much more flexible.

Fixes flutter#7031
@abarth
abarth force-pushed the text_controller3 branch from f225e8e to abd6da3 Compare April 1, 2017 05:02
@abarth
abarth merged commit ae89948 into flutter:master Apr 2, 2017
@abarth
abarth deleted the text_controller3 branch April 2, 2017 00:30
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 13, 2021
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.

Need to rationalise the names of the *Input* and *Field* classes

3 participants