Skip to content

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Dec 5, 2018

Closes #18081

Text labels were previously hard coded to align in the center of a multiline text field:

This PR adds the alignLabelWithHint param to InputDecoration that 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.

@justinmc justinmc self-assigned this Dec 5, 2018
@justinmc justinmc requested a review from HansMuller December 5, 2018 18:52
const Curve _kTransitionCurve = Curves.fastOutSlowIn;

/// Alignment before focus, relevant to multiline inupts
enum TextFieldLabelAlignment {
Copy link
Contributor Author

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?

Copy link
Contributor

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

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Dec 5, 2018
@justinmc justinmc changed the title WIP InputDecorator label alignment param InputDecorator label alignment param Dec 5, 2018
@justinmc justinmc force-pushed the text-form-field-label-align branch from a899d91 to 110c0c7 Compare December 5, 2018 20:56
@justinmc
Copy link
Contributor Author

justinmc commented Dec 5, 2018

@HansMuller This is ready for a real review now. Let me know if I can elaborate on documentation or anything as well.

Copy link
Contributor

@HansMuller HansMuller left a 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 {
Copy link
Contributor

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);
Copy link
Contributor

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

/// 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
Copy link
Contributor

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));
Copy link
Contributor

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).

@justinmc justinmc force-pushed the text-form-field-label-align branch from 064db5d to a08e628 Compare December 6, 2018 16:38
@justinmc
Copy link
Contributor Author

justinmc commented Dec 6, 2018

@HansMuller Ready for re-review.

Is this what you were looking for regarding the debugFillProperties comment? 6fd5ad4 It looks like InputDecoration has no debugFillProperties method, and InputDecorator doesn't directly have alignLabelWithHint. Let me know if I should add a debugFillProperties or anything like that.

@justinmc justinmc force-pushed the text-form-field-label-align branch from 06fbe5d to 6fd5ad4 Compare December 6, 2018 17:57
Copy link
Contributor

@HansMuller HansMuller left a 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);
Copy link
Contributor

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].
Copy link
Contributor

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);
Copy link
Contributor

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

NICE

Copy link
Contributor Author

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.

@HansMuller HansMuller changed the title InputDecorator label alignment param Add InputDecoration alignLabelWithHint parameter Dec 6, 2018
@justinmc
Copy link
Contributor Author

justinmc commented Dec 7, 2018

Ready to merge after the end of code freeze.

@justinmc justinmc merged commit 89fa4cc into flutter:master Dec 18, 2018
@justinmc justinmc deleted the text-form-field-label-align branch December 18, 2018 18:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

labelText of multiline TextFormField should be top aligned

4 participants