-
Notifications
You must be signed in to change notification settings - Fork 29.7k
fix: time selector separator in timepicker is not centered vertically #168441
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
…hour and minute selectors
… the reduced test set
Co-authored-by: Mitchell Goodwin <[email protected]>
Co-authored-by: Mitchell Goodwin <[email protected]>
Co-authored-by: Mitchell Goodwin <[email protected]>
This updates the Flutter minimum macOS version from 10.14 to 10.15 adds a migrator for existing apps, and updates our own examples, tests, and benchmark apps to 10.15. A follow-up patch will drop macOS 10.15 `@available` checks in the embedder. This is required in order to use Swift in the embedder and not need to bundle the Swift runtime libs in every app that uses Flutter. Swift stable ABI was introduced in macOS 10.14.4. As of March 2025, usage of macOS 10.14 is approximately 1.2~1.8% depending on source of statistics, see example public usage data here: https://gs.statcounter.com/macos-version-market-share/desktop/worldwide This patch makes the following changes: 1. Updates mac_deployment_target from 12.0 to 13.0. 2. Changes templates to `MACOSX_DEPLOYMENT_TARGET`, `MinimumOSVersion`, and Podfile `platform :osx` to 10.15. 3. Adds migrator for Podfile part to migrate `platform :osx, '10.14'` -> `platform :osx, '10.15'` 4. Compiles with `-mmacosx-version-min=10.15` 5. Runs the migrator on all example apps and integration tests. 6. Updates examples, tests to macOS 10.15 deployment target Issue: flutter#167745 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This reverts commit f524fe3.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
MitchellGoodwin
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.
Apologies it took so long to take a look, but I think I know everything that is going on. Most of the inconsistencies where solved by other PRs, but in the tests it looks like the positioning of the separators change depending on whether it's a dial or input style time picker. It looks like two easy changes can be made to this PR to get them to look better.
The separator is positioned a little lower relative to the other widgets for the dial picker. Both widgets have the separator in the middle of a Row, but the input one adds a 10 pixel bottom padding to the other members of the Row. If you add a 10 pixel bottom padding around the _TimeSelectorSeparator for the input that should make the two types consistent with each other at least.
But also the separator sits a little too low than expected in between the two. I think that's from the font adding too much space on top, making it off-centered. It's ok if it's a little low, but I think the effect is too strong. I think this can be improved by changing the height of the effectiveTextStyle used for the separator.
final TextStyle effectiveStyle = MaterialStateProperty.resolveAs<TextStyle>(
timePickerTheme.timeSelectorSeparatorTextStyle?.resolve(states) ??
timePickerTheme.hourMinuteTextStyle ??
defaultTheme.timeSelectorSeparatorTextStyle?.resolve(states) ??
defaultTheme.hourMinuteTextStyle,
states,
).copyWith(color: effectiveTextColor, height: 1.0);Sorry for the delay again. This took some detective work. I think with those two changes this should be pretty smooth to merge in from there.
Thank you for the detailed feedback! For the _TimeSelectorSeparator padding: Please let me know if this is the correct approach, or if you’d like the padding applied differently. |
At the code line I linked to, it should only ever be in one of the input modes, so that conditional check is unnecessary. You can just wrap it with the padding. The expanded widgets next to it also have the same padding. |
|
Hi @MitchellGoodwin, The test is still failing in CI/CD, but I’m unable to reproduce the failure locally. I ran a full flutter analyze and flutter pub outdated — there are some outdated and discontinued packages, but no analysis issues locally. However, in the CI log, I see the following repeated error: There’s no explicit error message, just an empty -- This line intentionally left blank --. Could you please help me figure out what might be causing this CI-only analysis failure? Any insights or suggestions for debugging this would be really helpful. Thank you so much for your help! |
|
It's complaining about the formatting in two of the files: Found 2 Dart files which were formatted incorrectly.
To fix, run `dart format packages/flutter/test/material/time_picker_test.dart packages/flutter/lib/src/material/time_picker.dart` or:
git apply <<DONE
diff --git a/packages/flutter/test/material/time_picker_test.dart b/packages/flutter/test/material/time_picker_test.dart
index c300eb9dcc4..00000000000 100644
--- a/packages/flutter/test/material/time_picker_test.dart
+++ b/packages/flutter/test/material/time_picker_test.dart
@@ -1485,57 +1485,59 @@ void main() {
semantics.dispose();
});
- testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async {
- tester.view.physicalSize = const Size(400, 800);
- tester.view.devicePixelRatio = 1.0;
- addTearDown(tester.view.reset);
+ testWidgets(
+ 'TimePicker dialog displays centered separator between hour and minute selectors',
+ (WidgetTester tester) async {
+ tester.view.physicalSize = const Size(400, 800);
+ tester.view.devicePixelRatio = 1.0;
+ addTearDown(tester.view.reset);
- await tester.pumpWidget(
- MaterialApp(
- theme: ThemeData(useMaterial3: false),
- home: const MediaQuery(
- data: MediaQueryData(),
- child: Material(
- child: TimePickerDialog(
- initialTime: TimeOfDay(hour: 12, minute: 0),
+ await tester.pumpWidget(
+ MaterialApp(
+ theme: ThemeData(useMaterial3: false),
+ home: const MediaQuery(
+ data: MediaQueryData(),
+ child: Material(
+ child: TimePickerDialog(initialTime: TimeOfDay(hour: 12, minute: 0)),
),
),
),
- ),
- );
- await tester.pumpAndSettle();
+ );
+ await tester.pumpAndSettle();
- await expectLater(
- find.byType(Dialog),
- matchesGoldenFile('m2_time_picker.dialog.separator.alignment.png'),
- );
- });
+ await expectLater(
+ find.byType(Dialog),
+ matchesGoldenFile('m2_time_picker.dialog.separator.alignment.png'),
+ );
+ },
+ );
- testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async {
- tester.view.physicalSize = const Size(400, 800);
- tester.view.devicePixelRatio = 1.0;
- addTearDown(tester.view.reset);
+ testWidgets(
+ 'TimePicker dialog displays centered separator between hour and minute selectors',
+ (WidgetTester tester) async {
+ tester.view.physicalSize = const Size(400, 800);
+ tester.view.devicePixelRatio = 1.0;
+ addTearDown(tester.view.reset);
- await tester.pumpWidget(
- MaterialApp(
- theme: ThemeData(useMaterial3: true),
- home: const MediaQuery(
- data: MediaQueryData(),
- child: Material(
- child: TimePickerDialog(
- initialTime: TimeOfDay(hour: 12, minute: 0),
+ await tester.pumpWidget(
+ MaterialApp(
+ theme: ThemeData(useMaterial3: true),
+ home: const MediaQuery(
+ data: MediaQueryData(),
+ child: Material(
+ child: TimePickerDialog(initialTime: TimeOfDay(hour: 12, minute: 0)),
),
),
),
- ),
- );
- await tester.pumpAndSettle();
+ );
+ await tester.pumpAndSettle();
- await expectLater(
- find.byType(Dialog),
- matchesGoldenFile('m3_time_picker.dialog.separator.alignment.png'),
- );
- });
+ await expectLater(
+ find.byType(Dialog),
+ matchesGoldenFile('m3_time_picker.dialog.separator.alignment.png'),
+ );
+ },
+ );
testWidgets('provides semantics information for text fields', (WidgetTester tester) async {
final SemanticsTester semantics = SemanticsTester(tester);
@@ -2720,3 +2722,4 @@ Future<void> finishPicker(WidgetTester tester) async {
await tester.tap(find.text(materialLocalizations.okButtonLabel));
await tester.pumpAndSettle(const Duration(seconds: 1));
}
diff --git a/packages/flutter/lib/src/material/time_picker.dart b/packages/flutter/lib/src/material/time_picker.dart
index df565c34f83..00000000000 100644
--- a/packages/flutter/lib/src/material/time_picker.dart
+++ b/packages/flutter/lib/src/material/time_picker.dart
@@ -523,7 +523,7 @@ class _TimeSelectorSeparator extends StatelessWidget {
style: effectiveStyle,
textScaler: TextScaler.noScaling,
),
- )
+ ),
),
);
}
@@ -3837,3 +3837,4 @@ class _TimePickerDefaultsM3 extends _TimePickerDefaults {
// dart format on
// END GENERATED TOKEN PROPERTIES - TimePicker
DONE |
|
Other than the formatting complaints, code LGTM. |
Reference Link #168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
…1569) Reference Link flutter#168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
…flutter#168441) PR Description This PR addresses an issue in the Flutter time picker where the separator between the hour and minute selectors (:) was not centered vertically. The change ensures that the separator is now vertically centered, improving the overall UI alignment and consistency. Previously, the separator used only the TextAlign.center property, which did not fully address the vertical alignment issue. The updated code wraps the separator text in a Center widget to ensure proper centering both horizontally and vertically. Before: The separator (:) was horizontally centered but not aligned vertically. After: The separator is now both horizontally and vertically centered within the time picker.   Issues Fixed This PR fixes issue flutter#157402 (Vertical alignment issue in the time picker separator). --------- Co-authored-by: Mitchell Goodwin <[email protected]> Co-authored-by: Chris Bracken <[email protected]>
…1569) Reference Link flutter#168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
|
Just as ref: the low position from the separator probably is due to the wrong font size: |
…1569) Reference Link flutter#168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
…1569) Reference Link flutter#168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
…flutter#168441) PR Description This PR addresses an issue in the Flutter time picker where the separator between the hour and minute selectors (:) was not centered vertically. The change ensures that the separator is now vertically centered, improving the overall UI alignment and consistency. Previously, the separator used only the TextAlign.center property, which did not fully address the vertical alignment issue. The updated code wraps the separator text in a Center widget to ensure proper centering both horizontally and vertically. Before: The separator (:) was horizontally centered but not aligned vertically. After: The separator is now both horizontally and vertically centered within the time picker.   Issues Fixed This PR fixes issue flutter#157402 (Vertical alignment issue in the time picker separator). --------- Co-authored-by: Mitchell Goodwin <[email protected]> Co-authored-by: Chris Bracken <[email protected]>
…1569) Reference Link flutter#168441 (comment) This PR removes explicit ThemeData(useMaterial3: true) assignments from test files. Since useMaterial3 now defaults to true in Flutter, these lines are no longer necessary and can be safely omitted. **Why?** - Keeps test code clean and up-to-date with current Flutter defaults. - Prevents confusion for contributors who may think this setting is still required.
PR Description
This PR addresses an issue in the Flutter time picker where the separator between the hour and minute selectors (:) was not centered vertically. The change ensures that the separator is now vertically centered, improving the overall UI alignment and consistency.
Previously, the separator used only the TextAlign.center property, which did not fully address the vertical alignment issue. The updated code wraps the separator text in a Center widget to ensure proper centering both horizontally and vertically.
Before:
The separator (:) was horizontally centered but not aligned vertically.
After:
The separator is now both horizontally and vertically centered within the time picker.