-
Notifications
You must be signed in to change notification settings - Fork 29.7k
fix: time selector separator in timepicker is not centered vertically #157402
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
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. 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. |
|
@pranavo72bex thank you for putting together this PR. What issue is this fixing? The linked issue is the placeholder, and I couldn't find the issue by searching. Also this will need a regression test to ensure your fix isn't overwritten in the future. If there is a simple reproduction of the issue, then we can just test that and see if it's centered. If that's not possible, at worst we can add a golden file test since it looks like the time picker doesn't have one yet. It's fairly straight forward to do so, though kind of odd if you aren't used to it. This PR adds a similar golden test for a different widget, though ignore the leak tracking and scale. More in depth on goldens here: https://github.com/flutter/flutter/blob/master/docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md#creating-a-new-golden-file-test |
Thank you for reviewing the PR!. Here is an example code snippet and a preview to help illustrate the fix
|
|
Thank you for the reproduction. I think for this PR, it'd be best to add a golden test as mentioned in my last comment. Can you do so? |
Thank you for the feedback! I've added a golden test as requested to verify the corrected alignment of the separator (:) in the TimePickerDialog. The test you provided is structured as follows: This test captures the TimePickerDialog in Material 2 mode (useMaterial3: false) to verify that the separator (:) between the hour and minute selectors is now centered both horizontally and vertically, resolving the previous alignment issue. The generated golden file (time_picker.dialog.separator.alignment.corrected.png) reflects this corrected positioning for consistent UI verification. |
|
Analyzer is complaining about needing to be marked as a part of the reduced test set. Please add to the top of the file, above the library and In the Material tests, the file floating_action_button_test.dart is setup the same way. |
Is this not an issue in Material 3? |
Thank you for you feedback. Please verify the change 🙏 |
|
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. |
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.
Sorry I let this sit for a bit, it slipped out of my review queue.
I have a small nit on golden file naming, but other than that, this is good to go.
It is also affecting Material 3 too as shown in screenshot below
If it's affecting both, for completion you could add a Material 3 test as well easily by just copying your test, turning on the useMaterial3 and naming the other golden test the same as the other, but starting with m3.
|
|
||
| await expectLater( | ||
| find.byType(Dialog), | ||
| matchesGoldenFile('time_picker.dialog.separator.alignment.corrected.png'), |
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.
| matchesGoldenFile('time_picker.dialog.separator.alignment.corrected.png'), | |
| matchesGoldenFile('m2_time_picker.dialog.separator.alignment.png'), |
We try and have tests for Material 2 named as such. It makes looking them up later much easier.
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.
Thank you for you feedback. Please verify the changes 🙏
|
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. |
| }); | ||
| testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async { |
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: please add a line between the two tests. Also, it looks like the indentation of the first line of the new test is off.
Other than that, looks good.
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.
Thank you for your feedback. Could you kindly review the change 🙏
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.
Thank you for the update. Looks like the test formatting is still a little off. Also, could you please rebase this PR? The analyzer is complaining.
| }); | ||
|
|
||
| testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async { | ||
| tester.view.physicalSize = const Size(400, 800); |
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: body of this test is indented a little too much.
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.
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.
One of the checks was failing for the branch falling behind, so all @Piinks did was use the rabase button to catch up the PR. The indentation was not touched. I feel like the analyzer is not complaining about this PR, so rebasing again may be a good idea.
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 attempted to rebase the branch again, but the checks are still failing. Would you be able to take over this PR and resolve the issue? Please let me know if you need any context or assistance from my side.
Thank you for your help 🙏
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 attempted to rebase the branch again, but the checks are still failing. Would you be able to take over this PR and resolve the issue? Please let me know if you need any context or assistance from my side.
Thank you for your help 🙏
I think the last comment I left should fix the checks, then we can still merge it under your name. If that doesn't work and you want somebody to take over after that, we can still do that.
b34fc91 to
b5f4a81
Compare
|
Rebased to fix some of the failing checks. 👍 |
7ea7385 to
0c3c328
Compare
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.
Did some digging. The problem was that the resized testing view was persisting sometimes to other tests, and this test is expecting the view to be a certain width. So adding a addTearDown(tester.view.reset) to both tests should fix the checks. Then this will be good to go.
|
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. |
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.
LGTM! Thank you for being so responsive to my nits. This will need a second approval. I'll ask around to see about getting another reviewer.
dkwingsmt
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.
LGTM
|
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) |
|
Rebased for a fresh result from Google testing, cc @MitchellGoodwin |
2e8c1cb to
f524fe3
Compare
f524fe3 to
11c3067
Compare
|
Thank you to everyone who engaged with this pull request over the past several months. I initially opened this PR with the hope of contributing to the improvement of the Flutter TimePicker, specifically addressing the vertical alignment of the separator (":"), and followed through on feedback regarding golden tests and formatting. However, after six months without resolution or further progress—and despite multiple iterations, rebases, and ongoing follow-ups—I’ve decided to close this PR and revert the associated changes on my end. I genuinely appreciate the early feedback and engagement, but I must admit I’m quite disappointed by the prolonged delays and lack of final decision-making. I value this community and remain hopeful that future contributions will be met with clearer collaboration and resolution timelines. Thank you again. |
|
Apologies @pranavo72bex, I'll take full responsibility on this. This change was unusually tricky to diagnose the behavior in Google testing compared to the relatively straightforward change this PR was making. Combined with my other work this was difficult to prioritize, but I should have gave it more time. And at bare minimum I should have communicated better. My apologies. |
Thank you for your thoughtful response, @MitchellGoodwin — I really appreciate your honesty and the context. To help move things forward more efficiently, would it be possible for me to submit a separate PR that only includes the core fix (ensuring the separator is vertically centered) and leaves out the golden tests for now? I can then follow up with a second PR that focuses solely on adding the golden tests, which we can iterate on independently without blocking the main UI improvement. Thanks again for your support! |
The Google testing failures were because of golden tests that already existed within Google. The newly added one that was a part of this PR was not a problem. You are still more than welcome to reopen this PR. |
I have reopen the PR #168441 |
…#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 #157402 (Vertical alignment issue in the time picker separator). --------- Co-authored-by: Mitchell Goodwin <[email protected]> Co-authored-by: Chris Bracken <[email protected]>
…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]>
…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]>






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.
Pre-launch Checklist
I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
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.
All existing and new tests are passing.
For further assistance, you can reach out on the #hackers-new channel on [Discord].