Conversation
…ating navigation and refactoring existing date picker sample.
… and improve code consistency
- Add PickerStateTest with 6 tests for generic state management - Add TimePickerStateTest with 12 tests covering 12/24-hour formats - Add YearMonthPickerStateTest with 14 tests for year/month state - Add PickerUtilsTest with 11 tests for utility calculations Test coverage improved from 16 to 59 tests (+43 tests) Supports 1.0.0 stability goals 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…els and improved semantics
📝 WalkthroughWalkthroughThis PR introduces a new DatePicker component with synchronized year/month/day selection, refactors the Picker component with centralized styling through PickerDefaults and PickerColors/PickerTextStyles, converts time utility values to functions for reactive updates, and updates existing pickers (YearMonthPicker, TimePicker) to use the new styling system. The sample app is updated to showcase the new DatePicker. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DatePicker as DatePicker
participant DatePickerState as DatePickerState
participant Pickers as Year/Month/Day Pickers
participant Validation as validate()
User->>DatePicker: Selects Year or Month
DatePicker->>Pickers: Updates yearState or monthState
Pickers->>DatePickerState: State change detected
DatePickerState->>Validation: LaunchedEffect triggers
Validation->>Validation: Check maxDay for new year/month
alt Day exceeds new maxDay
Validation->>DatePickerState: Clamp dayState to maxDay
DatePickerState->>Pickers: Refresh day items and scroll
else Day is valid
Validation->>DatePickerState: No adjustment needed
end
DatePickerState->>User: Display updated selectedYear/<br/>selectedMonth/selectedDay
User->>DatePicker: Selects Day
DatePicker->>Pickers: Updates dayState
Pickers->>DatePickerState: selectedDay changes
DatePickerState->>User: Display updated date
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly Related PRs
Suggested Labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
139-139: Fix thecurrentDate()function call syntax in YearMonthPickerExample.Line 139 shows
currentDate.yearwithout parentheses, butcurrentDateis a function and must be called withcurrentDate(). This is inconsistent with the correct usage in the DatePickerExample (line 112) and the actual function definition in TimeUtil.kt.Fix required
- initialYear = currentDate.year, - initialMonth = currentDate.monthNumber + initialYear = currentDate().year, + initialMonth = currentDate().monthNumbersample/src/commonMain/kotlin/com/kez/picker/sample/App.kt (1)
50-59: Flag naming inconsistency between routes and screens.The route-to-screen mapping has become semantically misaligned:
Screen.DatePicker.route→YearMonthPickerSampleScreenScreen.DayPicker.route→DatePickerSampleScreenThis may confuse developers expecting route names to match their destination screens. Consider either:
- Renaming the routes to match their screens (e.g.,
Screen.YearMonthPickerandScreen.DatePicker), or- Swapping the screen assignments to restore semantic alignment.
🧹 Nitpick comments (12)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BackgroundStylePickerScreen.kt (1)
54-60: Refactor to avoid multiple function calls and improve consistency.Lines 54-55 call
currentDate()twice, which could theoretically lead to inconsistent year/month values if the date changes between calls (e.g., during a month/year boundary at midnight). Additionally,currentHouris stored in a local variable whilecurrentMinute()is called inline, creating inconsistency.For consistency with IntegratedPickerScreen.kt (lines 74-76) and YearMonthPickerSampleScreen.kt (line 47), consider caching all current time values:
🔎 Suggested refactor
+ val currentDate = currentDate() + val currentHour = currentHour() + val currentMinute = currentMinute() val yearMonthState = rememberYearMonthPickerState( - initialYear = currentDate().year, - initialMonth = currentDate().month.number + initialYear = currentDate.year, + initialMonth = currentDate.month.number ) - val currentHour = currentHour() val timeState = rememberTimePickerState( initialHour = currentHour, - initialMinute = currentMinute(), + initialMinute = currentMinute, initialPeriod = if (currentHour >= 12) TimePeriod.PM else TimePeriod.AM, timeFormat = TimeFormat.HOUR_12 )datetimepicker/src/commonTest/kotlin/com/kez/picker/PickerStateTest.kt (2)
7-14: KDoc mentions "State updates" but no corresponding tests exist.The documentation states tests cover "State updates," but all tests only verify initial value assignments. Consider adding a test that modifies
selectedItemand verifies the change is reflected, or update the KDoc to accurately describe the current coverage.
51-57: Test name may be misleading.
pickerState_sameInitialValue_areEqualtests thatselectedItemvalues are equal, not that the state instances themselves are equal. This is fine behavior-wise, but consider renaming topickerState_sameInitialValue_haveEqualSelectedItemsfor clarity.sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/DatePickerSampleScreen.kt (1)
69-86:clip()applied without a visible background.The
Boxhasclip(RoundedCornerShape(16.dp))but nobackground()modifier, so the clip has no visual effect. Either add a background color beforeclip()or remove the clip modifier.🔎 Proposed fix
Box( modifier = Modifier .fillMaxWidth() - .clip(RoundedCornerShape(16.dp)) .padding(24.dp) ) {Or add a background:
Box( modifier = Modifier .fillMaxWidth() + .background(MaterialTheme.colorScheme.surfaceVariant, RoundedCornerShape(16.dp)) .clip(RoundedCornerShape(16.dp)) .padding(24.dp) ) {datetimepicker/src/commonTest/kotlin/com/kez/picker/YearMonthPickerStateTest.kt (1)
148-192: Special date tests are redundant with earlier boundary tests.The "Special Date Tests" section (leap year, century year, end/start of year) largely duplicates earlier tests.
YearMonthPickerStatedoesn't have day-level logic, so testing "leap year" or "century year" here just verifies basic initialization, which is already covered by boundary tests. Consider removing these or clarifying what unique behavior they validate.datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePickerState.kt (2)
17-26: MultiplecurrentDate()calls in default parameters could return inconsistent values.If
rememberDatePickerState()is called exactly at midnight, the threecurrentDate()calls might span a date change, resulting in inconsistent initial values (e.g., Dec 31 year with Jan 1 month and day). Consider capturing the date once:🔎 Proposed fix
@Composable fun rememberDatePickerState( - initialYear: Int = currentDate().year, - initialMonth: Int = currentDate().month.number, - initialDay: Int = currentDate().day + initialYear: Int? = null, + initialMonth: Int? = null, + initialDay: Int? = null ): DatePickerState { + val now = currentDate() + val year = initialYear ?: now.year + val month = initialMonth ?: now.month.number + val day = initialDay ?: now.day - return remember(initialYear, initialMonth, initialDay) { - DatePickerState(initialYear, initialMonth, initialDay) + return remember(year, month, day) { + DatePickerState(year, month, day) } }
95-102: Silent fallback for invalid month values.The
else -> 31branch silently handles invalid month values (outside 1-12). Consider throwing anIllegalArgumentExceptionto fail fast, or add input validation in the constructor.🔎 Proposed fix
private fun daysInMonth(year: Int, month: Int): Int { + require(month in 1..12) { "Month must be in range 1-12, but was $month" } return when (month) { 1, 3, 5, 7, 8, 10, 12 -> 31 4, 6, 9, 11 -> 30 2 -> if (isLeapYear(year)) 29 else 28 - else -> 31 // Should not happen with 1-12 range + else -> error("Unreachable") // Now guarded by require above } }datetimepicker/src/commonTest/kotlin/com/kez/picker/date/DatePickerStateTest.kt (1)
51-63: Consider adding tests forvalidate()behavior after state changes.The current tests only verify initialization behavior. Since
validate()is called when year/month changes (perDatePicker.kt), consider adding tests that simulate post-initialization state changes to ensure day clamping works correctly during runtime.datetimepicker/src/commonMain/kotlin/com/kez/picker/date/YearMonthPicker.kt (1)
81-86: Consider addingstartLocalDateas a key torememberif dynamic updates are expected.The
yearStartIndexandmonthStartIndexare computed once and won't update ifstartLocalDatechanges after initial composition. If this is intentional (start indices only matter for initial positioning), consider adding a clarifying comment. Otherwise, addstartLocalDateas a key:val yearStartIndex = remember(startLocalDate) { yearItems.indexOf(startLocalDate.year) }datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (2)
53-60: Minor edge case:currentDate()is called twice in defaults.If
currentDate()is invoked across midnight,initialYearandinitialMonthcould theoretically be inconsistent (e.g., December 31st for one call, January 1st for the next). Consider computing the date once:🔎 Proposed fix
@Composable fun rememberYearMonthPickerState( - initialYear: Int = currentDate().year, - initialMonth: Int = currentDate().month.number + initialDate: LocalDate = currentDate(), + initialYear: Int = initialDate.year, + initialMonth: Int = initialDate.month.number ): YearMonthPickerState {This is an extremely rare edge case, so it's optional to address.
102-125: Same timing edge case applies to time picker defaults.
currentHour()andcurrentMinute()are called separately, which could result in inconsistent time if called across a minute boundary. This is a minor edge case.The
initialPeriodderivation frominitialHour(line 106) is correctly implemented.datetimepicker/src/commonMain/kotlin/com/kez/picker/time/TimePicker.kt (1)
90-109: Samerememberkey consideration asYearMonthPicker.The start index calculations won't update if
startTimechanges after initial composition. If this is intentional, consider adding a clarifying comment for maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
README.mddatetimepicker/src/commonMain/kotlin/com/kez/picker/Picker.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/PickerDefaults.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePicker.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePickerState.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/date/YearMonthPicker.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/time/TimePicker.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeCalculation.ktdatetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/PickerStateTest.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/PickerUtilsTest.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/TimePickerStateTest.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/YearMonthPickerStateTest.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/date/DatePickerStateTest.ktdatetimepicker/src/commonTest/kotlin/com/kez/picker/util/TimeCalculationTest.ktsample/src/commonMain/kotlin/com/kez/picker/sample/App.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/navigation/Screen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BackgroundStylePickerScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BottomSheetSampleScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/DatePickerSampleScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/HomeScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/IntegratedPickerScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/TimePickerSampleScreen.ktsample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/YearMonthPickerSampleScreen.kt
🧰 Additional context used
🧬 Code graph analysis (12)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BottomSheetSampleScreen.kt (2)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (3)
currentDate(52-52)currentHour(80-80)currentMinute(73-73)datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (1)
rememberYearMonthPickerState(52-60)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/TimePickerSampleScreen.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (2)
currentHour(80-80)currentMinute(73-73)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeCalculation.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDateTime(44-45)
datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePicker.kt (3)
datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePickerState.kt (1)
rememberDatePickerState(17-26)datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDate(52-52)datetimepicker/src/commonMain/kotlin/com/kez/picker/Picker.kt (1)
Picker(88-332)
datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (3)
currentDate(52-52)currentHour(80-80)currentMinute(73-73)
sample/src/commonMain/kotlin/com/kez/picker/sample/App.kt (1)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/YearMonthPickerSampleScreen.kt (1)
YearMonthPickerSampleScreen(42-121)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/YearMonthPickerSampleScreen.kt (4)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDate(52-52)datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (1)
rememberYearMonthPickerState(52-60)sample/src/commonMain/kotlin/com/kez/picker/sample/StringUtils.kt (1)
getMonthName(27-43)datetimepicker/src/commonMain/kotlin/com/kez/picker/date/YearMonthPicker.kt (1)
YearMonthPicker(53-134)
datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePickerState.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDate(52-52)
datetimepicker/src/commonMain/kotlin/com/kez/picker/time/TimePicker.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDateTime(44-45)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/IntegratedPickerScreen.kt (1)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (3)
currentDate(52-52)currentHour(80-80)currentMinute(73-73)
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BackgroundStylePickerScreen.kt (2)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (3)
currentDate(52-52)currentHour(80-80)currentMinute(73-73)datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (1)
rememberTimePickerState(102-125)
datetimepicker/src/commonMain/kotlin/com/kez/picker/date/YearMonthPicker.kt (2)
datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (1)
currentDate(52-52)datetimepicker/src/commonMain/kotlin/com/kez/picker/Picker.kt (1)
Picker(88-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: Linux • desktop
- GitHub Check: Linux • wasm
- GitHub Check: macOS • iOS Simulator Test
- GitHub Check: Linux • android
🔇 Additional comments (41)
datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerDefaults.kt (1)
21-102: LGTM! Well-designed defaults following Material3 patterns.The centralized defaults approach is excellent:
- Clear separation of concerns with
PickerColorsandPickerTextStyles- Appropriate use of
@Composablefor context-dependent defaults- Good use of
@Immutableannotation for data classes- Comprehensive documentation
datetimepicker/src/commonTest/kotlin/com/kez/picker/PickerUtilsTest.kt (1)
1-143: Excellent test coverage for picker utilities.The test suite comprehensively covers:
- Modulo operations for cyclic behavior (positive, negative, large values)
- Validation of odd/even visible item counts
- Infinite scroll multiplier bounds (explicitly testing < Int.MAX_VALUE)
- Start index calculations for both infinity and bounded modes
- Fraction coercion and edge cases
This aligns well with the Picker.kt refactor replacing
Int.MAX_VALUEwithINFINITE_SCROLL_MULTIPLIER.datetimepicker/src/commonMain/kotlin/com/kez/picker/Picker.kt (7)
109-115: Good input validation with clear error messages.The validation requirements ensure:
- Non-empty items list
- Positive odd
visibleItemsCountfor proper center alignment- Valid
startIndexrangeClear, actionable error messages help developers quickly identify issues.
58-63: Safer infinite scrolling with bounded multiplier.Replacing
Int.MAX_VALUEwithINFINITE_SCROLL_MULTIPLIER = 1000is a smart change that:
- Prevents potential memory issues and integer overflow
- Still provides a virtually infinite scrolling experience
- Aligns with the test expectations in PickerUtilsTest.kt
180-189: Excellent accessibility improvements with semantic labels.The picker-level semantics provide:
- Descriptive
contentDescriptionwith current selectionstateDescriptionfor screen readersLiveRegionMode.Politefor non-intrusive updatesCollectionInfofor structural navigationThis significantly enhances the accessibility experience.
262-283: Comprehensive per-item accessibility semantics.Each item now includes:
Role.Buttonfor proper interaction semantics- Context-aware
contentDescriptionwith picker label and selection stateselectedflag for current selectionCollectionItemInfofor position contextThis provides excellent support for screen readers and assistive technologies.
228-228: Good practice: Shared InteractionSource reduces allocations.Using a single shared
MutableInteractionSourcefor all clickable items is more efficient than creating individual instances. Sinceindication = nullis specified, ripple effects are disabled, making the shared source safe to use across all items.
143-147: Safe item retrieval with modulo arithmetic.The
getItemfunction correctly:
- Handles empty lists with null return
- Uses
mod()for safe cyclic indexing- Uses
getOrNull()for bounds safetyThis prevents index out of bounds errors in the infinite scrolling logic.
155-171: Consider documenting the expected behavior when custom TextStyle objects have unspecified font properties.The item height calculation defensively handles unspecified
lineHeightandfontSizeby falling back to0.sp. WithPickerDefaults.textStyles(), this is not a concern—bothfontSize(16.sp and 22.sp) andlineHeight(inherited fromLocalTextStyle.current) are specified. However, if a caller passes customTextStyleobjects with neither property explicitly set,textHeightSpcould be0.sp, resulting in minimal item heights (only padding). Either document this requirement or consider usingTextStyledefaults more defensively.datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeCalculation.kt (1)
32-40: Clean refactor using currentDateTime() function.The change consolidates date/time extraction into a single
currentDateTime()call, which:
- Improves code clarity and maintainability
- Ensures consistent timestamp across year/month/day extraction
- Aligns with the broader TimeUtil API evolution in this PR
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/navigation/Screen.kt (1)
8-8: LGTM!The new DayPicker navigation route follows the established pattern and integrates cleanly with the existing Screen sealed class.
datetimepicker/src/commonTest/kotlin/com/kez/picker/util/TimeCalculationTest.kt (2)
19-19: LGTM!Extracting
currentDayupfront aligns with the existing pattern forcurrentYearandcurrentMonth, improving consistency.
33-33: LGTM!The usage of
currentDayis consistent across all test cases and maintains test correctness.Also applies to: 54-54, 75-75, 96-96
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/BottomSheetSampleScreen.kt (2)
62-62: LGTM!The migration to the new function-based date/time API (
currentDate(),currentHour(),currentMinute()) is clean and aligns with the PR's refactoring objectives. The local variable assignments ensure these values are captured at composition time.Also applies to: 67-68
63-66: LGTM!The picker state initialization correctly uses the captured date/time values and properly derives the initial period based on the hour value.
Also applies to: 69-74
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/TimePickerSampleScreen.kt (2)
57-58: LGTM!Capturing the current time values upfront using
currentHour()andcurrentMinute()ensures both picker states are initialized with consistent values.
60-69: LGTM!Both 12-hour and 24-hour TimePicker states are correctly initialized using the captured time values, with proper AM/PM period calculation for the 12-hour format.
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/IntegratedPickerScreen.kt (1)
74-86: LGTM! Clean refactor to function-based time accessors.The local variables ensure consistent state by calling each function once and reusing the result, avoiding potential edge cases where values might change during initialization.
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/HomeScreen.kt (1)
82-95: LGTM! Menu structure updated correctly.The renamed "YearMonthPicker Sample" and new "DayPicker Sample" menu items are properly configured with correct navigation routes and icons.
datetimepicker/src/commonTest/kotlin/com/kez/picker/TimePickerStateTest.kt (1)
1-203: Excellent test coverage for TimePickerState!The test suite comprehensively covers:
- 24-hour and 12-hour format initialization
- Critical edge cases (midnight, noon, end of day)
- Minute boundaries (0 and 59)
- State independence between multiple instances
- TimeFormat property validation
Well-structured and follows testing best practices.
sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/YearMonthPickerSampleScreen.kt (1)
47-121: LGTM! Well-implemented sample screen.The screen demonstrates proper state management by caching
currentDate()once and usingderivedStateOffor reactive computed values. The UI composition follows Compose best practices with clear separation of concerns.sample/src/commonMain/kotlin/com/kez/picker/sample/ui/screen/DatePickerSampleScreen.kt (1)
36-40: Function signature changed from internal to public without default parameter.The function is now public and
onBackPressedno longer has a default value. This is a breaking change for any existing callers that relied on the default. Verify this is intentional.datetimepicker/src/commonTest/kotlin/com/kez/picker/YearMonthPickerStateTest.kt (1)
1-193: Good test coverage overall.The test suite thoroughly covers initialization, boundaries, and state independence. The structure with section comments improves readability.
datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePickerState.kt (1)
39-79: Well-designed state encapsulation with automatic day validation.The
DatePickerStateclass properly encapsulates internal picker states while exposing read-only accessors. Theinitblock correctly clamps the initial day, and the leap year logic is accurate.datetimepicker/src/commonTest/kotlin/com/kez/picker/date/DatePickerStateTest.kt (1)
7-103: Solid test coverage for initialization and leap year logic.The tests thoroughly cover
maxDaycalculations, initial day clamping, and leap year edge cases including century rules. The parameterized approach intestMaxDay_AllMonthsandtestMaxDay_February_LeapYearVariationsis clean.datetimepicker/src/commonMain/kotlin/com/kez/picker/util/TimeUtil.kt (2)
37-45: Good refactor: Converting properties to functions prevents stale time values.The documentation clearly explains the rationale. This change ensures time-dependent defaults in picker states are always current when accessed.
54-80: Helper functions correctly derive fromcurrentDateTime().Each function properly computes its value from
currentDateTime(), ensuring consistency and freshness. The API is clean and well-documented.datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePicker.kt (2)
72-75: Good: State validation on year/month changes.The
LaunchedEffectcorrectly triggersvalidate()when year or month changes, ensuring the day is clamped to valid bounds (e.g., Feb 30 → Feb 28).
104-169: Well-structured picker layout with accessibility labels.The Row layout with weighted pickers and
pickerLabelfor accessibility is well-implemented. Disabling infinite scroll for the day picker (isInfinity = false) is correct since day count varies by month.datetimepicker/src/commonMain/kotlin/com/kez/picker/date/YearMonthPicker.kt (4)
20-27: LGTM! Import updates align with the new styling system.The imports correctly bring in
PickerColors,PickerDefaults, andPickerTextStylesto support the consolidated styling API.
58-72: Well-structured parameter defaults using centralized PickerDefaults.The migration to
PickerDefaultsfor default values provides consistent styling across picker components and simplifies customization.
101-111: Accessibility enhancement withpickerLabelis well implemented.The addition of
pickerLabel = "Year"andpickerLabel = "Month"(line 129) provides proper context for screen readers, aligning with the accessibility improvements in the broader Picker component.
150-171: Preview functions effectively demonstrate the new styling API.The updated previews showcase how to use
PickerDefaults.colors()andPickerDefaults.textStyles()for customization, serving as good documentation for API consumers.datetimepicker/src/commonMain/kotlin/com/kez/picker/PickerState.kt (3)
28-42: Good encapsulation withinternal setforselectedItem.Making the setter internal ensures that only the
Pickercomponent can update the selected item through scrolling, preventing external code from causing inconsistent state. This is a sound API design choice.
71-90: Well-designed encapsulation forYearMonthPickerState.Internalizing
yearStateandmonthStatewhile exposing read-onlyselectedYearandselectedMonthgetters provides a clean public API and prevents external code from corrupting the picker's internal state.
138-168: Consistent encapsulation pattern applied toTimePickerState.The internal state visibility with public read-only getters mirrors the
YearMonthPickerStatedesign. The KDoc comments are informative, especially noting the hour range differences between 12-hour and 24-hour formats.datetimepicker/src/commonMain/kotlin/com/kez/picker/time/TimePicker.kt (5)
22-24: LGTM! Import updates consistent with the styling system refactor.
63-81: Consistent styling defaults applied fromPickerDefaults.The parameter defaults align with the
YearMonthPickerand follow the centralized styling pattern established in the PR.
117-135: Period picker correctly configured withisInfinity = false.The AM/PM picker appropriately disables infinite scrolling since there are only two options. The
pickerLabel = "Period"provides good accessibility context.
138-174: Hour and Minute pickers properly updated with accessibility labels.Both pickers include
pickerLabelfor screen reader context and use the new styling parameters consistently.
205-228: Preview functions effectively demonstrate the new styling API.The
TimePickerCustomColorsPreviewandTimePickerLargeTextPreviewprovide good examples of customization usingPickerDefaults.colors()andPickerDefaults.textStyles().
| modifier: Modifier = Modifier, | ||
| pickerModifier: Modifier = Modifier, | ||
| state: DatePickerState = rememberDatePickerState(), | ||
| startLocalDate: LocalDate = currentDate(), |
There was a problem hiding this comment.
Unused parameter: startLocalDate.
The startLocalDate parameter is declared but never used in the function body. Either remove it or use it for initial index calculations as the KDoc suggests.
🔎 Proposed fix - remove unused parameter
@Composable
fun DatePicker(
modifier: Modifier = Modifier,
pickerModifier: Modifier = Modifier,
state: DatePickerState = rememberDatePickerState(),
- startLocalDate: LocalDate = currentDate(),
yearItems: List<Int> = YEAR_RANGE,
monthItems: List<Int> = MONTH_RANGE,Also update the KDoc to remove the @param startLocalDate line.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePicker.kt around
line 56, the constructor/function parameter startLocalDate is declared but never
used; remove the unused startLocalDate parameter from the signature, delete its
@param entry from the KDoc, and update any call sites to stop passing that
argument (or, alternatively, if intended to set the initial index, wire
startLocalDate into the initial index calculation where currentDate() is used);
ensure the code compiles and tests/builds after removing the parameter and
updating the documentation and callers.
| // To ensure 1:1 mapping with Picker's internal state on first render: | ||
| val yearStartIndex = remember { yearItems.indexOf(state.selectedYear) } | ||
| val monthStartIndex = remember { monthItems.indexOf(state.selectedMonth) } | ||
| // Day items change dynamically, so we can't fully pre-calculate a static list and index | ||
| // without being careful. | ||
|
|
||
| // Dynamic day items based on maxDay | ||
| val maxDay = state.maxDay | ||
| val dayItems = (1..maxDay).toList() | ||
| // Ensure selected day index is valid for the current dayItems | ||
| val dayStartIndex = remember(dayItems) { | ||
| val index = dayItems.indexOf(state.selectedDay) | ||
| if (index >= 0) index else 0 | ||
| } |
There was a problem hiding this comment.
yearStartIndex and monthStartIndex are not reactive to external state changes.
Using remember {} without keys means these indices are computed only once. If the DatePickerState is externally modified or reused with different initial values, the pickers won't scroll to the correct position.
🔎 Proposed fix
- val yearStartIndex = remember { yearItems.indexOf(state.selectedYear) }
- val monthStartIndex = remember { monthItems.indexOf(state.selectedMonth) }
+ val yearStartIndex = remember(state.selectedYear, yearItems) {
+ yearItems.indexOf(state.selectedYear).coerceAtLeast(0)
+ }
+ val monthStartIndex = remember(state.selectedMonth, monthItems) {
+ monthItems.indexOf(state.selectedMonth).coerceAtLeast(0)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // To ensure 1:1 mapping with Picker's internal state on first render: | |
| val yearStartIndex = remember { yearItems.indexOf(state.selectedYear) } | |
| val monthStartIndex = remember { monthItems.indexOf(state.selectedMonth) } | |
| // Day items change dynamically, so we can't fully pre-calculate a static list and index | |
| // without being careful. | |
| // Dynamic day items based on maxDay | |
| val maxDay = state.maxDay | |
| val dayItems = (1..maxDay).toList() | |
| // Ensure selected day index is valid for the current dayItems | |
| val dayStartIndex = remember(dayItems) { | |
| val index = dayItems.indexOf(state.selectedDay) | |
| if (index >= 0) index else 0 | |
| } | |
| // To ensure 1:1 mapping with Picker's internal state on first render: | |
| val yearStartIndex = remember(state.selectedYear, yearItems) { | |
| yearItems.indexOf(state.selectedYear).coerceAtLeast(0) | |
| } | |
| val monthStartIndex = remember(state.selectedMonth, monthItems) { | |
| monthItems.indexOf(state.selectedMonth).coerceAtLeast(0) | |
| } | |
| // Day items change dynamically, so we can't fully pre-calculate a static list and index | |
| // without being careful. | |
| // Dynamic day items based on maxDay | |
| val maxDay = state.maxDay | |
| val dayItems = (1..maxDay).toList() | |
| // Ensure selected day index is valid for the current dayItems | |
| val dayStartIndex = remember(dayItems) { | |
| val index = dayItems.indexOf(state.selectedDay) | |
| if (index >= 0) index else 0 | |
| } |
🤖 Prompt for AI Agents
In datetimepicker/src/commonMain/kotlin/com/kez/picker/date/DatePicker.kt around
lines 89 to 102, the yearStartIndex and monthStartIndex are currently wrapped in
remember { ... } with no keys so they are computed only once and won’t update
when DatePickerState changes; update the remembers to include reactive keys (at
minimum state.selectedYear and yearItems for yearStartIndex, and
state.selectedMonth and monthItems for monthStartIndex) so the indices are
recomputed when relevant state or item lists change, keeping the dayStartIndex
logic as-is.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new DatePicker component for selecting year, month, and day with automatic day validation, while refactoring the existing picker API to use a more consistent and accessible design pattern following Material3 guidelines. The changes include comprehensive test coverage and updated documentation.
Key changes:
- Added
DatePickercomponent withDatePickerStatefor complete date selection with leap year and month-length validation - Refactored picker API to use
PickerDefaults.colors()andPickerDefaults.textStyles()for consistent styling across all picker components - Enhanced accessibility with semantic labels, content descriptions, selection states, and collection info for screen readers
- Converted time utility properties to functions to prevent stale values in long-running applications
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| YearMonthPickerSampleScreen.kt | New sample screen demonstrating standalone YearMonthPicker usage |
| TimePickerSampleScreen.kt | Updated to use function-based time utilities instead of properties |
| IntegratedPickerScreen.kt | Updated to use function-based time/date utilities |
| HomeScreen.kt | Added new menu items for YearMonthPicker and DayPicker samples |
| DatePickerSampleScreen.kt | Completely rewritten to showcase full DatePicker with year, month, and day selection |
| BottomSheetSampleScreen.kt | Updated to use function-based utilities and removed unused import |
| BackgroundStylePickerScreen.kt | Updated to use function-based utilities |
| Screen.kt | Added DayPicker route for navigation |
| App.kt | Updated routing to wire up YearMonthPickerSampleScreen and DatePickerSampleScreen |
| TimeCalculationTest.kt | Refactored variable naming for clarity (currentDate → currentDay) |
| DatePickerStateTest.kt | New comprehensive test suite for DatePickerState covering leap years, month validation, and day clamping |
| YearMonthPickerStateTest.kt | New comprehensive test suite for YearMonthPickerState |
| TimePickerStateTest.kt | New comprehensive test suite for TimePickerState covering 12/24 hour formats |
| PickerUtilsTest.kt | New test suite for picker utility functions and calculations |
| PickerStateTest.kt | New test suite for generic PickerState |
| TimeUtil.kt | Converted time/date properties to functions with improved documentation |
| TimeCalculation.kt | Updated to use function-based time utilities |
| TimePicker.kt | Refactored to use PickerDefaults, added accessibility labels, renamed parameters |
| YearMonthPicker.kt | Refactored to use PickerDefaults and added accessibility labels |
| DatePickerState.kt | New state class for DatePicker with validation and leap year logic |
| DatePicker.kt | New DatePicker component supporting year, month, and day selection |
| PickerState.kt | Made selectedItem setter internal, changed sub-state visibility to internal, updated to use function-based utilities |
| PickerDefaults.kt | New defaults object providing colors, text styles, and default values following Material3 patterns |
| Picker.kt | Major refactoring with accessibility enhancements, API changes, input validation, and infinite scroll improvements |
| README.md | Added DatePicker documentation and examples, updated API tables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var selectedItem: T by mutableStateOf(initialItem) | ||
| internal set |
There was a problem hiding this comment.
The setter for selectedItem is now internal, which prevents external modification. However, this could be a breaking API change for existing users who might have been setting this property directly. Consider documenting this breaking change in the PR description or migration guide if this is intentional.
| role = Role.Button, | ||
| indication = null, | ||
| interactionSource = remember { MutableInteractionSource() }, | ||
| interactionSource = sharedInteractionSource, |
There was a problem hiding this comment.
The shared MutableInteractionSource is created but not effectively utilized. Each clickable item still creates its own interaction behavior. If the intention is to share interaction state across items, this needs to be restructured. If not, this variable is unnecessary.
| interactionSource = sharedInteractionSource, | |
| interactionSource = remember { MutableInteractionSource() }, |
| val selectedLineHeight = selectedTextStyle.lineHeight.takeIf { it.isSpecified } ?: 0.sp | ||
| val unselectedLineHeight = textStyle.lineHeight.takeIf { it.isSpecified } ?: 0.sp | ||
| val largestLineHeight = | ||
| if (selectedLineHeight > unselectedLineHeight) selectedLineHeight else unselectedLineHeight | ||
|
|
||
| val selectedFontSize = selectedTextStyle.fontSize.takeIf { it.isSpecified } ?: 0.sp | ||
| val unselectedFontSize = textStyle.fontSize.takeIf { it.isSpecified } ?: 0.sp | ||
| val largestFontSize = | ||
| if (selectedFontSize > unselectedFontSize) selectedFontSize else unselectedFontSize | ||
|
|
||
| val textHeightSp = if (largestLineHeight > 0.sp) largestLineHeight else largestFontSize |
There was a problem hiding this comment.
The calculation of textHeightSp uses largestLineHeight if specified, otherwise falls back to largestFontSize. However, when lineHeight is not specified, it defaults to 0.sp which could lead to incorrect height calculations. Consider using the font size directly when line height is not specified, or using a more appropriate fallback value.
| val selectedLineHeight = selectedTextStyle.lineHeight.takeIf { it.isSpecified } ?: 0.sp | |
| val unselectedLineHeight = textStyle.lineHeight.takeIf { it.isSpecified } ?: 0.sp | |
| val largestLineHeight = | |
| if (selectedLineHeight > unselectedLineHeight) selectedLineHeight else unselectedLineHeight | |
| val selectedFontSize = selectedTextStyle.fontSize.takeIf { it.isSpecified } ?: 0.sp | |
| val unselectedFontSize = textStyle.fontSize.takeIf { it.isSpecified } ?: 0.sp | |
| val largestFontSize = | |
| if (selectedFontSize > unselectedFontSize) selectedFontSize else unselectedFontSize | |
| val textHeightSp = if (largestLineHeight > 0.sp) largestLineHeight else largestFontSize | |
| val selectedEffectiveTextHeight = when { | |
| selectedTextStyle.lineHeight.isSpecified && selectedTextStyle.lineHeight > 0.sp -> | |
| selectedTextStyle.lineHeight | |
| selectedTextStyle.fontSize.isSpecified && selectedTextStyle.fontSize > 0.sp -> | |
| selectedTextStyle.fontSize | |
| else -> 0.sp | |
| } | |
| val unselectedEffectiveTextHeight = when { | |
| textStyle.lineHeight.isSpecified && textStyle.lineHeight > 0.sp -> | |
| textStyle.lineHeight | |
| textStyle.fontSize.isSpecified && textStyle.fontSize > 0.sp -> | |
| textStyle.fontSize | |
| else -> 0.sp | |
| } | |
| val textHeightSp = | |
| if (selectedEffectiveTextHeight > unselectedEffectiveTextHeight) { | |
| selectedEffectiveTextHeight | |
| } else { | |
| unselectedEffectiveTextHeight | |
| } |
| internal val yearState = PickerState(initialYear) | ||
| internal val monthState = PickerState(initialMonth) |
There was a problem hiding this comment.
The visibility of yearState and monthState has been changed to internal, making them inaccessible from external code. This is a breaking API change that prevents direct manipulation of individual picker states. If this is intentional to enforce encapsulation, it should be documented in the migration guide.
| internal val yearState = PickerState(initialYear) | |
| internal val monthState = PickerState(initialMonth) | |
| val yearState = PickerState(initialYear) | |
| val monthState = PickerState(initialMonth) |
| dividerShape: Shape = PickerDefaults.DividerShape, | ||
| isDividerVisible: Boolean = true, | ||
| isInfinity: Boolean = true, | ||
| pickerLabel: String? = null, |
There was a problem hiding this comment.
The parameter name pickerLabel is used for accessibility but could be more descriptive. Consider renaming it to accessibilityLabel or contentLabel to make its purpose clearer, especially since it's specifically for accessibility features like screen readers.
| fadingEdgeGradient = fadingEdgeGradient, | ||
| horizontalAlignment = horizontalAlignment, | ||
| itemTextAlignment = verticalAlignment, | ||
| verticalAlignment = verticalAlignment, |
There was a problem hiding this comment.
The itemTextAlignment parameter has been renamed to verticalAlignment which is clearer and more consistent. However, this is a breaking API change that should be documented in the PR description or changelog to help users migrate their code.
| val maxDay = state.maxDay | ||
| val dayItems = (1..maxDay).toList() | ||
| // Ensure selected day index is valid for the current dayItems | ||
| val dayStartIndex = remember(dayItems) { |
There was a problem hiding this comment.
Using remember(dayItems) for dayStartIndex may cause issues because dayItems is recreated every recomposition when maxDay changes. This could lead to the picker resetting to an unexpected index when the month changes. Consider using remember(state.selectedDay, dayItems.size) to be more explicit about the dependencies.
| val dayStartIndex = remember(dayItems) { | |
| val dayStartIndex = remember(state.selectedDay, dayItems.size) { |
| | Parameter | Description | Default | | ||
| |:--------------------|:----------------------------------------|:----------------------------| | ||
| | `state` | The state object to control the picker. | `rememberDatePickerState()` | | ||
| | `startLocalDate` | The initial date to set the picker to. | `currentDate` | |
There was a problem hiding this comment.
The comment references currentDate without parentheses, but the actual API requires currentDate() as a function call. This inconsistency in documentation could confuse users.
| internal val hourState = PickerState(initialHour) | ||
| internal val minuteState = PickerState(initialMinute) | ||
| internal val periodState = PickerState(initialPeriod) |
There was a problem hiding this comment.
The visibility of hourState, minuteState, and periodState has been changed to internal. This is a breaking API change that prevents external code from accessing these states directly. Document this change and provide guidance on how users should migrate if they were previously accessing these properties.
| /** | ||
| * Default spacing between pickers in composite components (e.g., TimePicker, DatePicker). | ||
| */ | ||
| val SpacingBetweenPickers: Dp = 0.dp |
There was a problem hiding this comment.
The default value for SpacingBetweenPickers is 0.dp, which removes spacing between picker columns. This differs from the previous default of 20.dp used in the old API. This change could affect the visual appearance of existing picker implementations and should be noted as a visual breaking change or the default should be reconsidered.
| val SpacingBetweenPickers: Dp = 0.dp | |
| val SpacingBetweenPickers: Dp = 20.dp |
This pull request introduces a new
DatePickercomponent and enhances the genericPickercomposable with improved accessibility, customization, and documentation. The key changes include the addition of theDatePicker, updates to state management, a more robust and accessible picker implementation, and improved usage documentation.New Component & Documentation
DatePickercomponent: Added a complete date picker supporting year, month, and day selection with automatic day validation. Updated the documentation to describe its usage and parameters, including state management viarememberDatePickerState. [1] [2] [3]Picker API & Accessibility Improvements
PickerAPI for clarity and flexibility: Replaced individual style/color parameters withPickerDefaults.colors()andPickerDefaults.textStyles(). Added input validation for parameters and improved calculation of item heights for better layout consistency. [1] [2]Usability & Customization
Int.MAX_VALUEwith a safer multiplier for infinite lists to prevent memory issues while maintaining a smooth user experience.PickerDefaults. [1] [2] [3] [4] [5] [6]These changes collectively improve the library's feature set, accessibility, and ease of use for developers and end users.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.