Repository navigation
Input refactoring: added an InputField widget - #6733
Conversation
HansMuller
changed the title
0e3a186 to
5ad9a6d
Compare
mpcomplete
left a comment
mpcomplete
left a comment
There was a problem hiding this comment.
Overall feedback for posterity: I don't think we've quite nailed the proper division between RawInput/InputField/Input. They all are vaguely similar and each adds a seemingly arbitrary amount of features on top of the thing below it. I'm also not sure we can hide RawInput as an internal-only thing, because we need to provide a non-material way to handle text input.
That said, this is a step along the path to greatness, and addresses a customer need, so I'm OK with it.
LGTM with nits addressed.
| key: inputKey, | ||
| validator: errorText | ||
| ) | ||
| key: inputKey, |
| onTap: () => _rawInputKey.currentState?.requestKeyboard(), | ||
| child: child | ||
| ) | ||
| return new GestureDetector( |
There was a problem hiding this comment.
Why another gesture detector? Doesn't the InputField widget handle it?
There was a problem hiding this comment.
This GestureDetector is as big as the entire Input widget, not just its InputField child.
| await tester.pumpWidget(builder()); | ||
|
|
||
| Future<Null> checkText(String testValue) async { | ||
| enterText('Hello World'); |
| await tester.pumpWidget(builder()); | ||
|
|
||
| Future<Null> checkText(String testValue) async { | ||
| enterText('Hello World'); |
There was a problem hiding this comment.
Sorry, I was sloppy and then I cut and pasted.
|
|
||
| export 'package:flutter/services.dart' show TextInputType; | ||
|
|
||
| class InputField extends StatefulWidget { |
There was a problem hiding this comment.
I'm not crazy about this name. Seems very easy to confuse with InputFormField. Though we should probably do a rename pass on all of [RawInput, Input, InputField, InputFormField], so maybe it's good enough for now.
There was a problem hiding this comment.
I'd like to do a rename pass as well. I think what we call Input could be called TextField, as it is in the Material spec. FormField could be FormTextField. What I've called InputField is a little harder, maybe TextFieldInput. Perhaps we should find a way to not expose RawInput at all.
|
I don't understand this split. What does InputField give us that RawInput doesn't? I thought the split was going to be such that Input's materialness would be factored out into a class that didn't know anything about text input, and then Input would become that class + RawInput. |
| ), | ||
| ]; | ||
|
|
||
| if (config.hintText != null && value.text.isEmpty) { |
There was a problem hiding this comment.
in particular, I would have expected the hint feature to be in the same class as the underline and label and so forth, rather than being specific to a text field subclass.
There was a problem hiding this comment.
InputField supports the selection menu and hints and is just big as a Text widget. That's what #6691 is asking for. InputField also deals with the focus key. RawInput doesn't do any of those things.
Input is padded bigger, even without the error and label parts and includes a divider, per the Material spec.
There was a problem hiding this comment.
Seems like RawInput should handle the focus stuff.
I can buy the argument that we need a widget between the opinion-less RawInput and the fully-fledged Material Input that adds the Material-like copy/paste selection logic.
There was a problem hiding this comment.
But I think hints should move out of here and up to Input. We're gonna want hints when we do the more generic non-text-input widget split.
There was a problem hiding this comment.
I agree that it would be better if Input could manage it's hintText overlay. I will try and address that, along with the general requirement to enable writing custom widgets that lay out as Input does, in phase 2.
3 participants
Added an InputField Widget. It just displays hintText and it's InputValue, it doesn't include a label or support for error text. The Input widget now just contains an InputField.
Fixes #6691