-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Fixes InputDecorator to work with textScaleFactor, fixes Material Design differences. #12595
Conversation
358b03c
to
ccab76b
Compare
@@ -368,9 +368,13 @@ class InputDecorator extends StatelessWidget { | |||
/// Defaults to false. | |||
final bool isEmpty; | |||
|
|||
/// The widget below this widget in the tree. | |||
/// The widget below this widget in the tree. Typically, this is an [EditableText] widget. |
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 don't think that's true. In fact, of the two (non-test) cases where we create an InputDecorator in our code base, neither uses EditableText here. One uses a RepaintBoundary that contains an EditableText, and the other uses a Row to do fancy stuff.
I think most people using this directly will be doing so because they want to create a widget that matches a text field but isn't one.
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.
OK, reverted.
218bebb
to
96b1bd5
Compare
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 looks good, just a few minor comments.
_cardModels = new List<CardModel>.generate( | ||
_cardModels.length, | ||
(int i) { | ||
_cardModels[i].height = _editable ? max(_cardHeights[i], 60.0) : _cardHeights[i]; |
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 this papering over the fact that we'll draw the textfield underline through the middle of the textfield if there isn't enough room?
Maybe we should draw the underline at all, if there isn't enough vertical. Maybe we should provide a constant (lib/src/material/constants.dart) like kMinTextFieldSize?
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.
Well, we won't do that anymore, it'll just overflow, so it's not really papering over anything. I'm mainly fixing the demo here to not overflow. It does mean that the behavior has changed, but the old behavior was wrong.
If they are overflowing their container now, then they can either turn on "isDense" in the InputDecoration, or give the text field a InputDecoration.collapsed decoration.
The height of the text field can't be a constant, unfortunately. If it's multi-line or the textScaleFactor is >1.0, or the child of the decorator isn't a text field at all, then it'll be wrong.
@@ -2,6 +2,8 @@ | |||
// Use of this source code is governed by a BSD-style license that can be | |||
// found in the LICENSE file. | |||
|
|||
import 'dart:math'; |
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.
Glad to see that you've checked this old manual test!
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.
Well, it's part of the Travis smoke tests, so it kind of had to be fixed...
|
||
double topPadding = isCollapsed ? 0.0 : (isDense ? 12.0 : 16.0); | ||
double topPadding = isCollapsed ? 0.0 : 2.0 * (isDense ? _kDensePadding : _kNormalPadding); |
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.
It's hard to understand why there's a factor of 2 here. Maybe add a comment where these constants are defined.
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.
OK, I've added some new constants to document it better.
|
||
final double topPaddingIncrement = labelTextHeight + (isDense ? _kDensePadding : _kNormalPadding) - 4.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.
This another example of holding the changes to a higher standard than the original code: what's the -4 for here? I think it's comment-worthy.
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 turns out to be just a measurement error on my part: it's the height of the label descender (at the default font size), so I can just remove it here and below.
style: subtextStyle, | ||
textAlign: textAlign, | ||
overflow: TextOverflow.ellipsis, | ||
final double linePadding = _kBottomBorderHeight + (isDense ? _kDensePadding : _kNormalPadding) - 4.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.
Same nit about -4.0 as above here.
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.
Removed
errorText ?? helperText, | ||
style: subtextStyle, | ||
textAlign: textAlign, | ||
overflow: TextOverflow.ellipsis, |
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
@@ -52,4 +53,190 @@ void main() { | |||
|
|||
expect(tester.element(find.byKey(key)).size, equals(const Size(800.0, 60.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.
Shouldn't we have a test that verifies that the divider (the text underline) appears when it's supposed and that it appears in the correct place?
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.
Good point. Added.
…ign differences. There were a number of differences with the Material Design spec, including several different padding values and underline thickness. This corrects that so that the decorator is in line with the Material Design spec now. Also, the decorator properly handles changes to the textScaleFactor, where before it would not re-layout when needed, painting the cursor and underline incorrectly. The decorator also now properly animates helper, error, and hint text when the textScaleFactor or input decoration properties change.
96b1bd5
to
fc65fa0
Compare
ddf5133
to
7b136c9
Compare
LGTM |
…rial Design differences. (flutter#12595)" This reverts commit 67cf791. Reverting because this causes scuba regressions that I'd like to address in another PR that is pending, but we'd like to roll Flutter.
…xes Material Design differences. (flutter#12595)" (flutter#12678)" This reverts commit 72dc7d9. Re-landing my InputDecorator changes so that I can land the character counter PR.
There were a number of differences with the Material Design spec, including
several different padding values and underline thickness. This corrects
that so that the decorator is in line with the Material Design spec now.
Also, the decorator properly handles changes to the textScaleFactor, where
before it would not re-layout when needed, painting the cursor and
underline incorrectly.
The decorator also now properly animates helper, error, and hint text when
the textScaleFactor or input decoration properties change.
Helper text is now properly displayed in dense mode, as the spec shows.
Before this change, it was never displayed in dense mode.
Fixes #12485