Allow empty initial time when using text input mode in showTimePicker dialog #170694
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Hi all, this is my first PR. Happy to make any changes if needed. |
github-actions
bot
added
framework
Contributor
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Thank you for putting this together. It's a little awkward when using this code as the initial date still shows as a placeholder, which might be confusing. I think the hint text for the field can be set to show nothing if this new property is true:
| /// | ||
| /// Useful when users prefer manual input without clearing pre-filled values. | ||
| /// Ignored in dial mode. | ||
| final bool? emptyInitialTime; |
Contributor
There was a problem hiding this comment.
All these properties should not be nullable, especially when null in this case would always mean the same as false. We can just have constructors default to false.
| final Icon? switchToTimerEntryModeIcon; | ||
|
|
||
| /// If true, input fields start empty in input mode. | ||
| final bool? emptyInitialTimeInInputMode; |
Contributor
There was a problem hiding this comment.
This feels a little verbose. Maybe something like emptyInitialInput? Or emptyInputPlaceholder?
Contributor
Author
There was a problem hiding this comment.
Thank you. Renamed to the emptyInitialInput
…-text-input-mode-in-show-time-picker
Contributor
Author
|
Link on short Gist to use in your Flutter project I've enhanced the code based on your suggestions. Thank you for the helpful comments @MitchellGoodwin |
Contributor
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Thank you for the update. I left a few small nits.
Comment on lines
2309
to
2310
| /// Improves UX by removing the need to delete default values before typing. | ||
| /// Skipped in dial mode. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Improves UX by removing the need to delete default values before typing. | |
| /// Skipped in dial mode. | |
| /// Has no effect in dial mode. |
Nit: Generally we try and avoid putting opinions into the documentation like, "do this for good UX".
Comment on lines
2133
to
2138
| final String? hintText = | ||
| focusNode.hasFocus | ||
| ? null | ||
| : widget.emptyInitialTime | ||
| ? '' | ||
| : _formattedValue; |
Contributor
There was a problem hiding this comment.
Suggested change
| final String? hintText = | |
| focusNode.hasFocus | |
| ? null | |
| : widget.emptyInitialTime | |
| ? '' | |
| : _formattedValue; | |
| final String? hintText = | |
| focusNode.hasFocus || widget.emptyInitialTime | |
| ? null | |
| : _formattedValue; |
I think this would do the same thing and be a little cleaner?
Contributor
Author
There was a problem hiding this comment.
Yes, that would be cleaner. I've just adjusted code!
Thank you @MitchellGoodwin
…initial-time-when-using-text-input-mode-in-show-time-picker
Enhanced comments Co-authored-by: Mitchell Goodwin <[email protected]>
…when-using-text-input-mode-in-show-time-picker' into 169131_allow-empty-initial-time-when-using-text-input-mode-in-show-time-picker # Conflicts: # packages/flutter/lib/src/material/time_picker.dart
Contributor
Author
|
Thanks @MitchellGoodwin for the review. All minor fixes done! |
MitchellGoodwin
approved these changes
Jul 10, 2025
Contributor
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
LGTM, but the ci.yaml is getting stuck on something, and I'm unable to rebase through github. Could you rebase this PR? Then I'll ask around for a second review.
Reverts flutter#171726 The failure is fixed in flutter#171801 and the task has been green ever since. <img width="275" alt="image" src="https://github.com/user-attachments/assets/f2fc18e6-7dcd-4017-a5c5-ade6988b0692" /> Follow up for flutter#171725
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) 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-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
…1874) In https://review.skia.org/1017296, Skia deleted some logic in skia.gni that dealt with Fuchsia. Flutter had been unintentionally depending on that to set FreeType integration, as well as Ganesh GL/Vulkan and ICU. This moves those flags into Flutter's BUILD.gn file for Skia, making the explicit and keeping the old behavior the same as the new behavior. I had to add an explicit dependency between the Fuchsia SkFontMgr and Freetype, which has been "just working" by accident before. See https://g-issues.skia.org/issues/427681157 for more. ## 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 `///`). - [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. - [ ] 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
…#171879)" (flutter#171897) <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) 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-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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 <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…flutter#171879)" (flutter#171897)" (flutter#171910) <!-- start_original_pr_link --> Reverts: flutter#171897 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: matanlurey <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Was not the reason for the test failure, it was an OOB tag change. <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: auto-submit[bot] <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: <!-- start_original_pr_link --> Reverts: flutter#171879 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: chinmaygarde <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: flutter#171896 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/packages@cba2e90...4a231ae 2025-07-08 [email protected] [video_player] Adds platform view support on macOS (flutter/packages#9576) 2025-07-08 [email protected] [camera] fix `CameraLensType` export (flutter/packages#9536) 2025-07-08 [email protected] [video_player] Move iOS/macOS to per-player-instance Pigeon APIs (flutter/packages#9529) 2025-07-08 [email protected] Roll Flutter from 28a4e85 to adffe24 (17 revisions) (flutter/packages#9580) 2025-07-08 [email protected] [google_sign_in] Don't crash a misconfigured iOS app (flutter/packages#9486) 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-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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 <!-- end_revert_body --> <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
Maybe it works both ways, but just to make sure.
<!-- 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 adds a more descriptive error message for cases when BorderRadiusDirectional is used in widgets whose ancestors aren't wrapped into Directionality. Fixes flutter#88644 ## 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]. - [ ] 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
These builders are experimental and should not be used in production. copied from ddm experiment
Follow up to flutter#171682 Depends on dart-lang/tools#2125 --------- Co-authored-by: Nate Biggs <[email protected]>
github-actions
bot
added
tool
…-text-input-mode-in-show-time-picker
github-actions
bot
removed
a: tests
…-text-input-mode-in-show-time-picker
Contributor
|
@azatech the commit history on this PR has gotten pretty crazy, and has pulled in a bunch of unrelated commits. Can you try and remove all commits that aren't yours? It might be easier to open a new PR with these commits. If so, tag me and I'll approve that one as well and we'll go from there. |
…-text-input-mode-in-show-time-picker
Contributor
|
I don't think anything can remove those extra actions from the github page other than filing a new PR unfortunately, and it's probably why the CLA bot is getting an internal error. Can you file a new PR with a clean change list and tag the original reviewers? |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added ability to allow empty initial time when using text input mode in showTimePicker dialog #169131
///).