[LG-5532] feat(time-input): Segment state utils #3385
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
… from specified time zones
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces utility functions for managing time input segment state, including validation, formatting, and conversion between 12-hour and 24-hour formats. The implementation handles timezone conversions, segment validation, and provides comprehensive test coverage for UTC date creation from time segments.
- Adds utilities for validating and formatting time segments (hour, minute, second)
- Implements conversion logic between 12-hour and 24-hour time formats
- Introduces timezone-aware UTC date creation from time segments
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/time-input/src/utils/shouldSetValue/shouldSetValue.ts |
Core logic determining when to update time input value based on segment state |
packages/time-input/src/utils/isSameUTCDayAndTime/isSameUTCDayAndTime.ts |
Utility for comparing two dates for equality in UTC |
packages/time-input/src/utils/isEverySegmentValueExplicit/isEverySegmentValueExplicit.ts |
Validates that all time segments have unambiguous values |
packages/time-input/src/utils/isEverySegmentValid/isEverySegmentValid.ts |
Checks if all time segments are within valid ranges |
packages/time-input/src/utils/isEverySegmentFilled/isEverySegmentFilled.ts |
Verifies that all time segments contain values |
packages/time-input/src/utils/getNewUTCDateFromSegments/getNewUTCDateFromSegments.ts |
Creates UTC date from time segments with timezone support |
packages/time-input/src/utils/getFormattedTimeSegmentsFromDate/getFormattedTimeSegmentsFromDate.ts |
Extracts and formats time segments from a date object |
packages/time-input/src/utils/getFormattedTimeSegments/getFormattedTimeSegments.ts |
Pads time segments to 2-digit format |
packages/time-input/src/utils/findUnitOptionByDayPeriod/findUnitOptionByDayPeriod.ts |
Locates the AM/PM unit option matching a day period |
packages/time-input/src/utils/doesSomeSegmentExist/doesSomeSegmentExist.ts |
Checks if any time segment has a value |
packages/time-input/src/utils/convert12hTo24h/convert12hTo24h.ts |
Converts 12-hour format to 24-hour format |
packages/date-utils/src/newUTCFromTimeZone/newUTCFromTimeZone.ts |
Creates UTC date from local time in specified timezone |
packages/time-input/src/utils/index.ts |
Exports all new time input utility functions |
packages/date-utils/src/index.ts |
Exports new UTC timezone conversion utility |
| Test files (*.spec.ts) | Comprehensive test coverage for all new utilities |
packages/time-input/src/utils/isEverySegmentFilled/isEverySegmentFilled.spec.ts
Outdated
Show resolved
Hide resolved
packages/time-input/src/utils/doesSomeSegmentExist/doesSomeSegmentExist.spec.ts
Outdated
Show resolved
Hide resolved
This was referenced Dec 15, 2025
Contributor
|
Size Change: +255 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
shaneeza
requested review from
TheSonOfThomp and
stephl3
and removed request for
a team
…orrect input validation
| * // returns new Date('2026-02-21T04:00:00Z') | ||
| * ``` | ||
| */ | ||
| export const newUTCFromTimeZone = ({ |
Collaborator
There was a problem hiding this comment.
Seems like this is doing the same thing as newTZDate
Collaborator
Author
There was a problem hiding this comment.
I think you're correct. I initially looked at this, and I thought it was doing something else.
You have this comment innewTZDate, // This API is less than perfect. Do you remember why you wrote that?
Collaborator
There was a problem hiding this comment.
The function signature of newUTC matches the signature of new Date() (i.e. both support an array of numbers, newUTC(2025, 12, 19) etc)
Needing to prefix newTZDate with the offset feels a bit clunky to me
i.e. newTZDate(-5, 2025, 12, 19) just looks kinda wrong. Nothing inherently bad about it though
Collaborator
There was a problem hiding this comment.
Your implementation supports IANA strings, so we could extend newTZDate to support that: newTZDate('America/New_York', 2025, 12, 19)
Collaborator
Author
There was a problem hiding this comment.
I prefer not to pass the offset number and would rather use a string. I agree that it looks kinda wrong. I can update this to either:
With the IANA string and date, I can find the offset using getTimezoneOffset from date-fns, and continue to use what you have.
OR
With the IANA string and date, use the zonedTimeToUtc util from date-fns.
Regardless of the approach, we still need to depend on date-fns. I think that 2 would be easier because we don't need to calculate the offset manually.
Comment on lines
19
to
32
| // if dayPeriod is AM and hour is 12, return 0 since 12 AM is 00:00 | ||
| if (dayPeriod === 'AM') { | ||
| if (hour === '12') { | ||
| return '0'; | ||
| } | ||
|
|
||
| // else return hour as-is | ||
| return hour; | ||
| } | ||
|
|
||
| // if dayPeriod is PM and hour is 12, return 12 since 12 PM is 12:00 | ||
| if (hour === '12') { | ||
| return '12'; | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| // if dayPeriod is AM and hour is 12, return 0 since 12 AM is 00:00 | |
| if (dayPeriod === 'AM') { | |
| if (hour === '12') { | |
| return '0'; | |
| } | |
| // else return hour as-is | |
| return hour; | |
| } | |
| // if dayPeriod is PM and hour is 12, return 12 since 12 PM is 12:00 | |
| if (hour === '12') { | |
| return '12'; | |
| } | |
| if(hour === '12') { | |
| // 12AM -> 0:00 | |
| // 12PM -> 12:00 | |
| return dayPeriod === 'AM' ? '0' : '12' | |
| } |
| * @param dayPeriod - The day period to use for the conversion (AM or PM) | ||
| * @returns The converted hour | ||
| */ | ||
| export const convert12hTo24h = (hour: string, dayPeriod: string) => { |
Collaborator
There was a problem hiding this comment.
Why is hour: string , and not number?
Collaborator
There was a problem hiding this comment.
Also, should dayPeriod be an enum or string literal?
Suggested change
| export const convert12hTo24h = (hour: string, dayPeriod: string) => { | |
| export const convert12hTo24h = (hour: string, dayPeriod: 'AM' | 'PM') => { |
| } | ||
|
|
||
| // else return hour + 12 | ||
| return `${parseInt(hour) + 12}`; |
Collaborator
There was a problem hiding this comment.
What if hour is
<= 0?> 12?- any other string?
| * @returns The select unit option. | ||
| */ | ||
| export const findUnitOptionByDayPeriod = ( | ||
| dayPeriod: string, |
Collaborator
There was a problem hiding this comment.
I feel like dayPeriod should be more explicitly typed (everywhere)
Collaborator
Author
There was a problem hiding this comment.
agree, will update
| ): UnitOption => { | ||
| const selectUnitOption = unitOptions.find( | ||
| option => option.displayName === dayPeriod, | ||
| ) as UnitOption; |
Collaborator
There was a problem hiding this comment.
unitOptions.find(...) might return undefined. We should handle that case here
| * // returns: { hour: '02', minute: '30', second: '00' } | ||
| * ``` | ||
| */ | ||
| export const getFormattedTimeSegments = (segments: TimeSegmentsState) => { |
Collaborator
There was a problem hiding this comment.
using the word formatted here makes me think it's supposed to be using a the formatter
| const { day, month, year } = dateValues; | ||
| const { hour, minute, second } = segments; | ||
|
|
||
| const convertedHour = is12HourFormat |
Collaborator
There was a problem hiding this comment.
nit: maybe be more explicit with the var name? converted to what?
| day1.getUTCHours() === day2.getUTCHours() && | ||
| day1.getUTCMinutes() === day2.getUTCMinutes() && | ||
| day1.getUTCSeconds() === day2.getUTCSeconds() && | ||
| day1.getUTCMilliseconds() === day2.getUTCMilliseconds() |
Collaborator
There was a problem hiding this comment.
nit: do we need to check milliseconds?
| * @param day2 - The second date to check | ||
| * @returns Whether the two dates are the same day and time in UTC | ||
| */ | ||
| export const isSameUTCDayAndTime = ( |
Collaborator
There was a problem hiding this comment.
We can probably use isEqual from date-fns
https://date-fns.org/v4.1.0/docs/isEqual
Collaborator
There was a problem hiding this comment.
if we need to explicitly check UTC, can we rename this isEqualUTC and move to date-utils?
| import { isEverySegmentValueExplicit } from '../isEverySegmentValueExplicit/isEverySegmentValueExplicit'; | ||
|
|
||
| /** | ||
| * Checks if the new date should be set. |
Collaborator
There was a problem hiding this comment.
Can we summarize the logic here?
…leafygreen-ui into LG-5532/segments-state-utils
…corresponding tests
…es for time conversion and selection
…ests for date comparison in UTC
…om time-input exports
…ove clarity and consistency
shaneeza
commented
Dec 19, 2025
Collaborator
Author
There was a problem hiding this comment.
Ignore this hook, it's being replaced in the next PR
…ty for improved clarity
Contributor
|
Coverage after merging LG-5532/segments-state-utils into LG-5532/segments-display-values will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
This PR introduces utility functions for managing time input segment state, including validation, formatting, and conversion between 12-hour and 24-hour formats. The implementation handles timezone conversions, segment validation, and provides comprehensive test coverage.
This PR is the third PR in a chain of PRs
🧪 How to test changes