Skip to content

Conversation

@Jaineel-Mamtora
Copy link
Contributor

@Jaineel-Mamtora Jaineel-Mamtora commented Aug 13, 2025

This PR aims to fix an open issue: #173621

Pre-launch Checklist

Screenshots

Please find below the before and after screenshots

  • Before
before_fixing_spacing
  • After
after_fixing_spacing

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 13, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an inconsistent horizontal spacing issue in the time picker for non-English languages by changing the crossAxisAlignment of the hour and minute columns to CrossAxisAlignment.stretch. This change appears to correctly align the labels under the time input fields, ensuring they are centered like the text fields above them, regardless of the label's text length. The fix is simple and effective. My only suggestion is to add a test to prevent future regressions for this UI fix.

Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
crossAxisAlignment: CrossAxisAlignment.stretch,
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This change correctly fixes the alignment of the hour and minute labels. To prevent future regressions, please consider adding a test case for this UI fix. A golden test in packages/flutter/test/material/time_picker_test.dart that uses a non-English locale (like Korean, as shown in the issue) would be ideal to verify that the labels are correctly centered.

Choose a reason for hiding this comment

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

Here we see the glorious AI code review, which hallucinates that the example language in the issue is Korean. Locale('es') is Spanish 👏

@bleroux
Copy link
Contributor

bleroux commented Aug 13, 2025

@Jaineel-Mamtora If possible, can you add before/after screenshots to make the review easier?

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #173706 at sha b517118

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Aug 13, 2025
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #173706 at sha a301cf5

@ManuelRauber
Copy link

@Jaineel-Mamtora Thanks for your effort. However, the baseline of the text does not fit. Now, the separator is below the text baseline. I think that fixing this issue is not just setting the stretch value.

Also, keep in mind that other languages have other separators, like . or h.

@Jaineel-Mamtora
Copy link
Contributor Author

Jaineel-Mamtora commented Aug 13, 2025

@Jaineel-Mamtora Thanks for your effort. However, the baseline of the text does not fit. Now, the separator is below the text baseline. I think that fixing this issue is not just setting the stretch value.

Also, keep in mind that other languages have other separators, like . or h.

@ManuelRauber, thanks for pointing them out. I'll try to fix the baseline issue.

Further, upon investigating, it seems like the baselines in the Material 3 design docs for the input and the separator are inconsistent (pardon me, if that's not the case). Do we want to make the baseline consistent?

Please check below screenshots for reference:

material_3_time_picker

Fig. 1: Material 3 Time Picker as per docs


visual_representation_of_consistent_baseline

Fig. 2: My perception of consistent baseline for Material 3 Time Picker


Please confirm which one should be considered as consistent.

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #173706 at sha a11088c

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #173706 at sha c78da92

@ManuelRauber
Copy link

@Jaineel-Mamtora Thanks for your effort. However, the baseline of the text does not fit. Now, the separator is below the text baseline. I think that fixing this issue is not just setting the stretch value.
Also, keep in mind that other languages have other separators, like . or h.

@ManuelRauber, thanks for pointing them out. I'll try to fix the baseline issue.

Further, upon investigating, it seems like the baselines in the Material 3 design docs for the input and the separator are inconsistent (pardon me, if that's not the case). Do we want to make the baseline consistent?

Please check below screenshots for reference:

material_3_time_picker Fig. 1: Material 3 Time Picker as per docs

visual_representation_of_consistent_baseline Fig. 2: My perception of consistent baseline for Material 3 Time Picker

Please confirm which one should be considered as consistent.

Indeed, according to the specs, at least in that screenshot, its also off. But since it's off in all of the screenshots, it might just be the intended way.

My personal taste would be figure 2, the real baseline. But since Flutter material should respect the material design docs, it may be better to go for figure 1 instead.

@Jaineel-Mamtora
Copy link
Contributor Author

Indeed, according to the specs, at least in that screenshot, its also off. But since it's off in all of the screenshots, it might just be the intended way.

My personal taste would be figure 2, the real baseline. But since Flutter material should respect the material design docs, it may be better to go for figure 1 instead.

Got it. Then, I think my current change should suffice. I checked for remaining separators, and the UI is looking consistent for them all. Please find below the screenshot for the separators:

hour-min '.' separator

Fig 1: hour-min '.' separator

hour-min 'h' separator

Fig 2: hour-min 'h' separator

hour-min ':' separator in 24 hours format

Fig 3: hour-min ':' separator in 24 hours format

I checked in Android (API-35), macOS, iOS (16 and 16 Pro Max). The UI looks consistent as shown above. Also, all the test cases are passing.

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Sep 17, 2025

Gentle nudge @MitchellGoodwin (from triage)

@MitchellGoodwin
Copy link
Contributor

@Jaineel-Mamtora thank you for putting together this PR! Apologies for the late review. The approach looks reasonable, can you resolve the merge conflicts and rebase? The linux analyze check looks unrelated. Once it's all green again, I'm going to run Google testing on this PR just to see the golden file tests for Google apps using Flutter. I suspect that there might be some failures there that might show some odd behavior with real life use cases that we would want to address here. We may want to add some padding to make sure that the stretch doesn't stretch too far.

@Jaineel-Mamtora Jaineel-Mamtora requested review from a team and jtmcdole as code owners September 24, 2025 19:00
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems labels Sep 24, 2025
@Jaineel-Mamtora Jaineel-Mamtora force-pushed the show-time-picker-hours-mins-alignment branch from ac8a00f to 8857d94 Compare October 30, 2025 16:27
@Jaineel-Mamtora
Copy link
Contributor Author

Jaineel-Mamtora commented Oct 30, 2025

Hi @QuncCccccc,

I checked, for the default locale, it has the same dimensions as mentioned in the material design docs. Please check below screenshot:

inspect element to find dimensions in default locate for time picker

In case of other locale, for example, Spanish locale (es), the Period selector container (AM/PM) goes away, so it takes the available horizontal space as shown in below screenshot:

inspect element to find dimensions in Spanish locate for time picker

Please let me know your thoughts on this.

@QuncCccccc
Copy link
Contributor

In case of other locale, for example, Spanish locale (es), the Period selector container (AM/PM) goes away, so it takes the available horizontal space

I see. Thanks for checking! I think this looks okay! LGTM!

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 1, 2025

autosubmit label was removed for flutter/flutter/173706, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 5, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Nov 5, 2025
Merged via the queue into flutter:master with commit 8892bc8 Nov 5, 2025
80 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 6, 2025
flutter/flutter@e5d5c01...c5e809a

2025-11-06 [email protected] Remove WindowingOwner.hasTopLevelWindows (flutter/flutter#178033)
2025-11-06 [email protected] Fix DropdownMenu escape key does not close the menu (flutter/flutter#178002)
2025-11-06 [email protected] Update documentation tool reference in image.dart (flutter/flutter#177782)
2025-11-06 [email protected] Roll Skia from 4eb2383d38f2 to 5c4e1352128f (5 revisions) (flutter/flutter#178094)
2025-11-06 [email protected] Revert "Refactor OverlayPortal semantics (#173005)" (flutter/flutter#178007)
2025-11-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix verified input test failure in CI (attempt 4) (#178018)" (flutter/flutter#178089)
2025-11-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[web] Unify Surface code between Skwasm and CanvasKit (#177138)" (flutter/flutter#178085)
2025-11-05 [email protected] Fix verified input test failure in CI (attempt 4) (flutter/flutter#178018)
2025-11-05 [email protected] fix: inconsistent horizontal spacing between hours and mins in time picker for non-english language (flutter/flutter#173706)
2025-11-05 [email protected] Roll Fuchsia Linux SDK from mpsxF1gd-jbKNvmpm... to cm88aTLui5yorSGYQ... (flutter/flutter#178074)
2025-11-05 [email protected] Fix(ios): Remove arm64 exclusion to support Xcode 26 simulators (flutter/flutter#177065)
2025-11-05 [email protected] Roll Skia from 2ff897e9b440 to 4eb2383d38f2 (18 revisions) (flutter/flutter#178070)
2025-11-05 [email protected] [web] Unify Surface code between Skwasm and CanvasKit (flutter/flutter#177138)
2025-11-05 [email protected] Update more missing ninja deps (flutter/flutter#178079)
2025-11-05 [email protected] Add ninja / cmake deps to failing tests (flutter/flutter#178054)
2025-11-05 [email protected] [web] Don't add webparagraph suite to CI (flutter/flutter#177681)
2025-11-05 [email protected] Fixing broken link in engine readme (flutter/flutter#177987)
2025-11-05 [email protected] Print reason for adb command failure in verified input test (attempt 3) (flutter/flutter#178005)
2025-11-05 [email protected] Fix `ReorderableList` items jumping when drag direction reverses mid-animation (flutter/flutter#173241)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `overflow_bar.0.dart‎` example (flutter/flutter#177813)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `data_table.1.dart‎` example (flutter/flutter#177812)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `switch.1.dart` example (flutter/flutter#177811)
2025-11-04 [email protected] Roll Skia from c89b6118266b to 2ff897e9b440 (6 revisions) (flutter/flutter#177999)
2025-11-04 [email protected] Fix verified input test in CI (attempt 2) (flutter/flutter#177961)
2025-11-04 [email protected] Replace rendering for solid color circles (both filled and stroked) to use SDFs (flutter/flutter#177482)
2025-11-04 [email protected] Roll Fuchsia Linux SDK from vxK5obzfr1X9P2kSh... to mpsxF1gd-jbKNvmpm... (flutter/flutter#177996)
2025-11-04 [email protected] Validate that platforms specified in .ci.yaml target names match the platforms defined in the platform_properties section (flutter/flutter#177523)
2025-11-04 [email protected] Roll Packages from 1a7075b to 3d926aa (6 revisions) (flutter/flutter#177998)
2025-11-04 [email protected] Remove dead code from snippet_generator (flutter/flutter#174440)

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 [email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
IvoneDjaja pushed a commit to IvoneDjaja/flutter that referenced this pull request Nov 22, 2025
…icker for non-english language (flutter#173706)

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

This PR aims to fix an open issue:
flutter#173621

## 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.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

## Screenshots

Please find below the _before_ and _after_ screenshots 

- Before
<img width="294" height="264" alt="before_fixing_spacing"
src="https://github.com/user-attachments/assets/ce519c72-cb5e-4f08-8976-abb01f9eb76d"
/>

- After
<img width="294" height="261" alt="after_fixing_spacing"
src="https://github.com/user-attachments/assets/f010cbbf-680d-495b-a028-16c7fbdb9a09"
/>


<!-- 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

---------

Co-authored-by: Tong Mu <[email protected]>
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
…icker for non-english language (flutter#173706)

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

This PR aims to fix an open issue:
flutter#173621

## 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.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

## Screenshots

Please find below the _before_ and _after_ screenshots 

- Before
<img width="294" height="264" alt="before_fixing_spacing"
src="https://github.com/user-attachments/assets/ce519c72-cb5e-4f08-8976-abb01f9eb76d"
/>

- After
<img width="294" height="261" alt="after_fixing_spacing"
src="https://github.com/user-attachments/assets/f010cbbf-680d-495b-a028-16c7fbdb9a09"
/>


<!-- 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

---------

Co-authored-by: Tong Mu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants