-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Factor out Input's layout: add InputContainer etc #6881
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
| const Duration _kTransitionDuration = const Duration(milliseconds: 200); | ||
| const Curve _kTransitionCurve = Curves.fastOutSlowIn; | ||
|
|
||
| /// A simple text field. |
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.
Add some more prose here describing what this means.
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [Input] |
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.
add a brief explanation of why you're pointing to these, as in * [Input], which does this and that.
| /// | ||
| /// Requires one of its ancestors to be a [Material] widget. | ||
| /// Displays the visual elements of a material design text field. This | ||
| /// widget's child is the input field widget itself. |
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 s/text/form/ here, so people think they can use this for things other than text 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.
Only the first line is visible in the dartdoc overview pages, and I'm worried these two sentences make it sounds text-field specific (the next sentence explains that it's not, but that won't be visible).
| // InputFormField below. | ||
| Input({ | ||
| /// * [InputField] | ||
| /// * [Input] |
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...
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [InputField] |
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...
| /// | ||
| /// By default, the input uses a keyboard appropriate for text entry. | ||
| // | ||
| // Note: If you change this constructor signature, please also update |
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.
s/Note://
| } | ||
| ), | ||
| ), | ||
| ]; |
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.
why is there a Builder in the build function above?
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.
This the comment that appears here. Maybe it's not clear?
// Since the focusKey may have been created here, defer building the
// RawInput until the focusKey's context has been set. This is necessary
// because the RawInput will check the focus, like Focus.at(focusContext),
// when it builds.
| _inputFieldKey.currentState?.requestKeyboard(); | ||
| }, | ||
| child: child, | ||
| child: new Builder( |
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.
why a Builder?
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.
I'll add this comment:
Since the focusKey may have been created here, defer building the InputContainer until the
focusKey's context has been set. This is necessary because we're passing the value of
Focus.at(focusKey.currentContext, ...) along to the InputContainer.
|
This is fantastic. |
|
I have only the most minor comments, mostly about docs. LGTM. |
The InputContainer widget arranges its child, hint text, label, and error text per the Material spec: https://material.google.com/components/text-fields.html. The Input widget is now a simple combination of InputField and InputContainer.
The InputContainer's layout depends on two boolean parameters that are not defined for Input:
focusedandisEmpty. They imply the location and text attributes for the input hint and label. The Input widget sets focus to true when it has the focus, and isEmpty to true when its value is the empty string.Fixes #6421