-
Notifications
You must be signed in to change notification settings - Fork 29.7k
fix: inconsistent horizontal spacing between hours and mins in time picker for non-english language #173706
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
fix: inconsistent horizontal spacing between hours and mins in time picker for non-english language #173706
Conversation
…icker for non-english language
|
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. |
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.
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, |
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 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.
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.
Here we see the glorious AI code review, which hallucinates that the example language in the issue is Korean. Locale('es') is Spanish 👏
…r non-english locale
|
@Jaineel-Mamtora If possible, can you add before/after screenshots to make the review easier? |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
|
@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 Also, keep in mind that other languages have other separators, like |
@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:
Please confirm which one should be considered as consistent. |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
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. |
|
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. |
|
Gentle nudge @MitchellGoodwin (from triage) |
|
@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. |
…hours-mins-alignment
ac8a00f to
8857d94
Compare
|
Hi @QuncCccccc, I checked, for the default locale, it has the same dimensions as mentioned in the material design docs. Please check below screenshot:
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:
Please let me know your thoughts on this. |
I see. Thanks for checking! I think this looks okay! LGTM! |
|
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. |
… in time picker for non-english language (flutter/flutter#173706)
… in time picker for non-english language (flutter/flutter#173706)
… in time picker for non-english language (flutter/flutter#173706)
… in time picker for non-english language (flutter/flutter#173706)
… in time picker for non-english language (flutter/flutter#173706)
… in time picker for non-english language (flutter/flutter#173706)
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
…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]>
…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]>









This PR aims to fix an open issue: #173621
Pre-launch Checklist
///).Screenshots
Please find below the before and after screenshots