-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add InputDecoration alignLabelWithHint parameter #24993
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
Add InputDecoration alignLabelWithHint parameter #24993
Conversation
| const Curve _kTransitionCurve = Curves.fastOutSlowIn; | ||
|
|
||
| /// Alignment before focus, relevant to multiline inupts | ||
| enum TextFieldLabelAlignment { |
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.
Is there an enum somewhere that I can reuse instead?
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.
You could use AlignmentGeometry and make the default value AlignmentDirectional.centerStart. The docs could attention to AlignmentDirection.topStart as a common alternative. Using AlignmentGeometry is a little more complicated and it adds a bunch of options that probably aren't needed.
This enum shouldn't really be called TextField... since it's for configuring InputDecorator.
Since it's really just a glorified flag, you could make it a flag. Like bool alignLabelWithHint
a899d91 to
110c0c7
Compare
|
@HansMuller This is ready for a real review now. Let me know if I can elaborate on documentation or anything as well. |
HansMuller
left a comment
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.
Please add a check for InputDecorator.debugFillProperties() and InputDecoration.debugFillProperties too.
| const Curve _kTransitionCurve = Curves.fastOutSlowIn; | ||
|
|
||
| /// Alignment before focus, relevant to multiline inupts | ||
| enum TextFieldLabelAlignment { |
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.
You could use AlignmentGeometry and make the default value AlignmentDirectional.centerStart. The docs could attention to AlignmentDirection.topStart as a common alternative. Using AlignmentGeometry is a little more complicated and it adds a bunch of options that probably aren't needed.
This enum shouldn't really be called TextField... since it's for configuring InputDecorator.
Since it's really just a glorified flag, you could make it a flag. Like bool alignLabelWithHint
| boxConstraints = boxConstraints.copyWith(maxWidth: inputWidth); | ||
| if (label != null) // The label is not baseline aligned. | ||
| label.layout(boxConstraints, parentUsesSize: true); | ||
| layoutLineBox(label); |
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.
The labelAlignment check will be needed here as well. If label alignment is the center (as before) then it's height doesn't contribute to the height of the prefix-hint-input-suffix baseline-aligned.
| } | ||
| if (label != null) | ||
| centerLayout(label, start - label.size.width); | ||
| if (label != null) { |
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.
Even though there are only two cases here, we consistently use switch to dispatch enum values. Here and below...
| /// If provided, this replaces the semantic label of the [counterText]. | ||
| final String semanticCounterText; | ||
|
|
||
| /// When the input is multiline (maxLines > 1) and before it is focused, the |
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 is tricky. It can be specified for any InputDecorator, even if the InputDecorator doesn't contain a multiline texfield.
You could say that it's typically specified when the decorator's child is a multline [TextField], i.e. a text field for which [TextField.maxLines] is null or > 1, to override the default center alignment for the label.
| ), | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
| expect(tester.getSize(find.byType(InputDecorator)), const Size(800.0, 56.0)); |
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.
Also test this against a multiline text field (the default child for buildInputDecorator is Text('text').
Rather than checking absolute top/left values, verify that the label is in the same location as the hint (other tests verify that the hint ends up in the right place).
064db5d to
a08e628
Compare
|
@HansMuller Ready for re-review. Is this what you were looking for regarding the |
06fbe5d to
6fd5ad4
Compare
HansMuller
left a comment
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.
LGTM, just needs some small changes
| if (decoration.alignLabelWithHint) { | ||
| layoutLineBox(label); | ||
| } else { | ||
| label.layout(boxConstraints, parentUsesSize: true); |
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.
In this case, best to preserve the comment FWIW
// The label is centered, not baseline aligned
|
|
||
| /// Typically set to true when the [InputDecorator] contains a multiline | ||
| /// [TextField] ([TextField.maxLines] is null or > 1) to override the default | ||
| /// behavior of aligning the label in the center of the [TextField]. |
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.
in the center => with the center
| this.alignLabelWithHint = false, | ||
| }) : assert(isDense != null), | ||
| assert(isCollapsed != null), | ||
| assert(filled != null); |
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.
Give this line one more indent space and add assert(alignLabelWithHint != null)
| properties.add(DiagnosticsProperty<InputBorder>('errorBorder', errorBorder, defaultValue: defaultTheme.errorBorder)); | ||
| properties.add(DiagnosticsProperty<InputBorder>('focusedBorder', focusedBorder, defaultValue: defaultTheme.focusedErrorBorder)); | ||
| properties.add(DiagnosticsProperty<InputBorder>('focusedErrorborder', focusedErrorBorder, defaultValue: defaultTheme.focusedErrorBorder)); | ||
| properties.add(DiagnosticsProperty<InputBorder>('focusedErrorBorder', focusedErrorBorder, defaultValue: defaultTheme.focusedErrorBorder)); |
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.
NICE
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.
My test already caught a mistake haha.
|
Ready to merge after the end of code freeze. |
Closes #18081
Text labels were previously hard coded to align in the center of a multiline text field:
This PR adds the
alignLabelWithHintparam toInputDecorationthat allows you to opt in to aligning the label to the top, in the same place that the hint and the first line of text will appear.