[time_picker] refactor: Distinguish widgets for dial mode only #173188
Conversation
|
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
bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several private widget names within the time picker to improve clarity, particularly for widgets used only in the dial entry mode. The new names like _TimePickerDialHeader and _DialHourControl are more descriptive. The removal of a redundant switch statement is also a good cleanup. However, the renaming of _HourControl and _MinuteControl will break existing tests in packages/flutter/test/material/time_picker_test.dart which look for these widgets by their old names. These tests need to be updated to reflect the new widget names.
Gustl22
force-pushed
the
dail-time-picker
branch
from
3f34d6d to
0958ecc
Compare
github-actions
bot
added
the
a: internationalization
|
I'm sorry I have not been able to review this PR. Will soon! |
Co-authored-by: Victor Sanni <[email protected]>
Co-authored-by: Victor Sanni <[email protected]>
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the cleanup!
# Conflicts: # packages/flutter/test/material/time_picker_test.dart
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
LGTM with a couple questions.
|
|
||
| return SizedBox( | ||
| height: height, | ||
| height: defaultTheme.hourMinuteSize.height, |
There was a problem hiding this comment.
Do we need to test that this value is never defaultTheme.hourMinuteInputSize.height?
There was a problem hiding this comment.
I don't think that's what we want.
defaultTheme.hourMinuteInputSize.height could be equal to defaultTheme.hourMinuteSize.height. I didn`t look up the values, but in any future material design version, this may is the case and should be tweakable (in my opinion). Besides that any change should be detectable through golden tests anyways.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| assert(_debugDialTimePickerEntryMode(context)); |
There was a problem hiding this comment.
Do we need to test the asserts (or they are never triggered)?
There was a problem hiding this comment.
You mean, if they actually work? I don't see a case there they can be triggered by the user as they are all present in a private class and only called internally in dial mode. We could open the classes for testing and intentionally set the wrong mode, if that's something what we want (?)
But also I don't see this very critical and only as help for contributors.
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
LGTM, thanks for the change.
victorsanni
added
the
autosubmit
|
Thank you for reviewing! |
flutter-dashboard
bot
removed
the
autosubmit
…10116) Manual roll requested by [email protected] flutter/flutter@b1a28bc...6cc976e 2025-09-25 [email protected] [Impeller] Optimize scale translate rectangle transforms (flutter/flutter#171841) 2025-09-25 [email protected] [time_picker] refactor: Distinguish widgets for dial mode only (flutter/flutter#173188) 2025-09-25 [email protected] Reapply "Update the AccessibilityPlugin::Announce method to account f… (flutter/flutter#174365) 2025-09-25 [email protected] Put Linux firebase_release_smoke_test on bringup (flutter/flutter#176043) 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
…er#173188) The code for time picker involves ambiguous widget names, which makes understanding the widget structure more challenging. I tried to use the namings of the [M3 specs](https://m3.material.io/components/time-pickers/specs) to make it a bit clearer. This also removes a code section which is not needed, as the widget is only meant for "dial" and not "input" mode. I didn't raise an issue, but I can, if that's still necessary. This PR is test-exempt, as it doesn't add or fix any functionality. ## 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]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] 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]. - [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]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- 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: Victor Sanni <[email protected]>
3 participants
The code for time picker involves ambiguous widget names, which makes understanding the widget structure more challenging.
I tried to use the namings of the M3 specs to make it a bit clearer.
This also removes a code section which is not needed, as the widget is only meant for "dial" and not "input" mode.
I didn't raise an issue, but I can, if that's still necessary.
This PR is test-exempt, as it doesn't add or fix any functionality.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.