-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Make CupertinoTextField
at least as tall as its first line of placeholder
#134198
Make CupertinoTextField
at least as tall as its first line of placeholder
#134198
Conversation
attachment: widget.clearButtonMode, | ||
hasText: text.text.isNotEmpty, | ||
); | ||
return switch (attachment) { |
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.
TIL you can use switch statements in this way. Awesome!
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 new in dart3. It's a switch expression (an expression evaluates to a value so you can directly return a switch expression), a bit different from a switch statement.
@@ -1134,6 +1111,31 @@ class _CupertinoTextFieldState extends State<CupertinoTextField> with Restoratio | |||
return _hasDecoration ? TextAlignVertical.center : TextAlignVertical.top; | |||
} | |||
|
|||
void _onClearButtonTapped() { | |||
// Special handle onChanged for ClearButton |
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.
consider consolidating this comment into one sentence. maybe something like Handler for ClearButton that calls onChanged when the clear button is tapped
.
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 basically moved the code and the comment to an instance method. I'll update the comment.
@@ -393,7 +393,7 @@ class CupertinoTextField extends StatefulWidget { | |||
this.onSubmitted, | |||
this.onTapOutside, | |||
this.inputFormatters, | |||
this.enabled, | |||
this.enabled = 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.
I wonder if we should just make this a private variable inside of CupertinoTextFieldState
similar to how TextFieldState
does it. I'm not too opinionated on this either way but it would keep the API consistent between both TextField
s.
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.
TextField.enabled?
is a bool?
because it can default to InputDecoration.enabled
I think. Here it doesn't have anything to default to other than the const true
so I don't know if there's a good reason to keep this bool?
.
I don't think we need to store enabled
in CupertinoTextFieldState since the configuration is completely determined by the widget itself (CupertinoTextField.enabled
), unless I'm missing something?
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 didn't consider the defaults in InputDecoration
, that makes sense to me then.
// Ideally this should be baseline aligned. However that comes at | ||
// the cost of the ability to compute the intrinsic dimensions of | ||
// this widget. | ||
// See also https://github.com/flutter/flutter/issues/13715 |
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.
nit: consider adding a period at the end of this line.
final String? placeholderText = widget.placeholder; | ||
final Widget? placeholder = placeholderText == null | ||
? null | ||
: Visibility( |
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.
Before this change the placeholder would not be built at all if there was text, but now it is built regardless but hidden when there is text. This is so the input field does not shrink when the user starts entering text because now the placeholder is still there just hidden.
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.
Just wondering if I am understanding this change correctly.
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.
Yeah the placeholder will be kept in the widget tree to make sure the text field's height is always greater than or equal to the height of the first line of the placeholder, even when there's user text.
…ng/flutter into taller-cupertino-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.
LGTM
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
flutter/flutter@aea4552...da676f7 2023-09-08 fluttergithubbot@gmail.com Marks Linux_android very_long_picture_scrolling_perf__e2e_summary to be unflaky (flutter/flutter#134116) 2023-09-08 31859944+LongCatIsLooong@users.noreply.github.com Make `CupertinoTextField` at least as tall as its first line of placeholder (flutter/flutter#134198) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from f09a139101c3 to 6d6b44886175 (3 revisions) (flutter/flutter#134306) 2023-09-08 gspencergoog@users.noreply.github.com Update links to iOS embedder docs to point to new Doxygen docs (flutter/flutter#134246) 2023-09-08 engine-flutter-autoroll@skia.org Roll Packages from 22d4754 to aaae5ef (7 revisions) (flutter/flutter#134301) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from c38feb382e96 to f09a139101c3 (1 revision) (flutter/flutter#134299) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from f146d4bf244b to c38feb382e96 (1 revision) (flutter/flutter#134293) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from a140ab4a4ec6 to f146d4bf244b (1 revision) (flutter/flutter#134290) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from ea1d0d28e26f to a140ab4a4ec6 (5 revisions) (flutter/flutter#134288) 2023-09-08 xubaolin@oppo.com fix a Scrollbar example crash (flutter/flutter#127925) 2023-09-08 tessertaha@gmail.com Fix `Drawer` examples are missing `dartpad` tag (flutter/flutter#134219) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4ac4429a206b to ea1d0d28e26f (1 revision) (flutter/flutter#134270) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from df3965a55fd8 to 4ac4429a206b (1 revision) (flutter/flutter#134267) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 505ef3c33102 to df3965a55fd8 (1 revision) (flutter/flutter#134263) 2023-09-08 polinach@google.com _TabBarViewState should dispose created instances of PageController. (flutter/flutter#134091) 2023-09-08 polinach@google.com Remove non needed controllers in SegmentedButton. (flutter/flutter#134064) 2023-09-08 polinach@google.com EditableTextState should dispose cursorVisibilityNotifier. (flutter/flutter#133858) 2023-09-08 polinach@google.com TestWidgetsFlutterBinding should dispose old RestorationManager on reset. (flutter/flutter#133999) 2023-09-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 65f6fb841c7a to 505ef3c33102 (4 revisions) (flutter/flutter#134260) 2023-09-07 polinach@google.com CupertinoAlertDialog should not create ScrollController on every build, if null values are passed in constructor. (flutter/flutter#134075) 2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from a828c26e7e97 to 65f6fb841c7a (1 revision) (flutter/flutter#134254) 2023-09-07 polinach@google.com _SearchBarState should dispose FocusNode, if it created it. (flutter/flutter#134076) 2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2dba8ceca824 to a828c26e7e97 (8 revisions) (flutter/flutter#134249) 2023-09-07 34871572+gmackall@users.noreply.github.com [integration_test] Allow capture of screenshots for `FlutterFragmentActivity`s (flutter/flutter#132406) 2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8d07c2947e60 to 2dba8ceca824 (2 revisions) (flutter/flutter#134243) 2023-09-07 tessertaha@gmail.com Fix `ExpansionTile` properties cannot be updated with `setState` (flutter/flutter#134218) 2023-09-07 737941+loic-sharma@users.noreply.github.com [Windows Arm64] Remove device_type property (flutter/flutter#134181) 2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from f0b718e28779 to 8d07c2947e60 (3 revisions) (flutter/flutter#134240) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
// In the middle part, stack the placeholder on top of the main EditableText | ||
// if needed. | ||
Expanded( | ||
child: Stack( | ||
// Ideally this should be baseline aligned. However that comes at |
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.
Since this change, the placeholder is vertically centered on multi-line textfield instead of being on the first line, because alignment is set to center instead of default topLeft. Is this intended? Not sure how to read the comment 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.
Yeah it should be baseline aligned with the input text instead of center aligned. I'll try to fix that.
Fixes #133241
and some CupertinoTextField cleanup.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.