fix: Make time picker seconds optional #347
Conversation
WalkthroughThe changes modify the time picker component to make the seconds field optional both in its data model and user interface. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShadTimePicker
participant ShadTimePickerController
participant ShadTimeOfDay
User->>ShadTimePicker: Interacts with Time Picker UI
ShadTimePicker->>ShadTimePicker: Checks showSeconds property
alt showSeconds = true
ShadTimePicker->>User: Displays hour, minute, and seconds fields
User->>ShadTimePicker: Enters hour, minute, second
ShadTimePicker->>ShadTimeOfDay: Constructs with hour, minute, second
else showSeconds = false
ShadTimePicker->>User: Displays hour and minute fields only
User->>ShadTimePicker: Enters hour, minute
ShadTimePicker->>ShadTimeOfDay: Constructs with hour, minute, second=0
end
ShadTimePicker->>ShadTimePickerController: Updates value
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/src/components/time_picker.dart (1)
1-1203: 💡 Verification agent🧩 Analysis chain
Consider adding test coverage for the new
showSecondsfunctionalityThe PR objectives mention that all existing and new tests pass, but no new tests were added specifically for this change. Consider adding tests that:
- Verify the time picker works correctly when
showSecondsis false- Validate focus management behavior in both cases
- Ensure value construction is correct with/without seconds
This would help ensure the feature remains stable with future changes.
🏁 Script executed:
#!/bin/bash # Search for existing tests related to time picker rg -A 2 -B 2 "ShadTimePicker|time_picker_test" --glob="**/*test*.dart"Length of output: 788
Add targeted tests for
showSecondsbehaviorAlthough there’s an existing golden test for
ShadTimePicker, we’re missing unit/widget tests that specifically cover the newshowSecondsflag. Please add tests to verify:
- The number of input fields changes when
showSecondsisfalse(no seconds field).- Focus traversal jumps correctly:
- From minutes → trailing widget or AM/PM selector when
showSeconds = false.- From minutes → seconds → AM/PM when
showSeconds = true.- The
onChangedcallback builds the correctShadTimeOfDay:
- When
showSeconds = false, seconds default to0.- When
showSeconds = true, seconds reflect the entered value.This will ensure the feature remains robust to future changes.
🧹 Nitpick comments (1)
lib/src/components/time_picker.dart (1)
819-823: Consider adding a comment explaining the focus management logicWhile the implementation is correct, a brief comment explaining the focus management logic would help future developers understand the intention behind this code.
if (v.length == 2 && effectiveJumpToNextField && widget.variant == ShadTimePickerVariant.period) { + // Move focus to period selector after entering seconds periodFocusNode.requestFocus(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/components/time_picker.dart(8 hunks)
🔇 Additional comments (8)
lib/src/components/time_picker.dart (8)
20-20: Good change: Madesecondparameter optional with default value of 0This change correctly makes the second parameter optional in the
ShadTimeOfDayconstructor, which aligns with the PR objective of making seconds optional in the time picker.
152-156: Change is aligned with the constructor updateRemoving the null check for
secondin the conditional return and defaulting it to 0 when constructing theShadTimeOfDayobject is consistent with the constructor changes.
240-240: Good implementation: AddedshowSecondsparameter to all constructorsThe
showSecondsparameter with a default value oftruehas been consistently added to all three constructor variants of theShadTimePickerwidget.Also applies to: 290-290, 336-336
339-342: Well-documented new propertyThe documentation for the new
showSecondsproperty is clear and follows the existing documentation style with template macros.
643-644: Correctly updated validation logicThe validation logic now only checks if
secondis null whenshowSecondsis true, which is consistent with making seconds optional.
654-654: Proper null handling for seconds valueUsing the null-coalescing operator to default
secondto 0 when it's null ensures consistent behavior when seconds are not specified.
793-797: Smart focus management implementationThe updated focus traversal logic correctly handles two cases:
- When seconds are shown, focus moves to the seconds field
- When seconds are hidden and the picker is in period mode, focus jumps directly to the period selector
This ensures a smooth user experience regardless of the
showSecondssetting.
802-825: Good UI implementation: Conditional rendering of seconds fieldThe seconds input field is now conditionally rendered based on the
showSecondsproperty, which completes the implementation of making seconds optional in the UI.
There was a problem hiding this comment.
@lucasbstn What about making each field optional? By default all are shown, but a show* is available for every input field
Adds a new showSeconds property to ShadTimePicker.
Most times, when I want to ask a user for a time, seconds are irrelevant.
Pre-launch Checklist
///).If you need help, consider asking for advice on Discord.
Summary by CodeRabbit