Repository navigation
feat(react-timepicker-preview): add time utilities - #36812
Victor Genaev (mainframev) wants to merge 1 commit into
Conversation
8d8509a to
9858197
Compare
|
Pull request demo site: URL |
| "compilerOptions": { | ||
| "noEmit": false, | ||
| "lib": ["ES2019", "dom"], | ||
| "lib": ["ES2019", "dom", "ES2020.Intl"], |
There was a problem hiding this comment.
same as in current compat package, needed because of the hourCycle option in formatDateToTimeString
0b67d38 to
63c083e
Compare
63c083e to
d98f1f0
Compare
d98f1f0 to
30f0f08
Compare
30f0f08 to
21f1de7
Compare
fd21697 to
0adf6b1
Compare
📊 Bundle size report✅ No changes found |
9283fb6 to
d22429e
Compare
d22429e to
cb29115
Compare
Paul Mardling (PaulGMardling)
left a comment
Paul Mardling (PaulGMardling)
left a comment
There was a problem hiding this comment.
3 more comments, one is quite an odd edge case
daf6eb8 to
b8e5f7d
Compare
Dmytro Kirpa (dmytrokirpa)
left a comment
Dmytro Kirpa (dmytrokirpa)
left a comment
There was a problem hiding this comment.
The main correctness issue is that localized formatter output cannot always be parsed back. I also suggest sharing the Intl formatter/configuration between formatting and parsing. Separately, the tree-shakeable Temporal function API is worth evaluating for DST resolution, provided we measure the actual bundle impact and verify compatibility; that is a non-blocking design suggestion, not a requirement to add a polyfill in this PR.
80b15e9 to
220b752
Compare
220b752 to
4b9dc4c
Compare
Paul Mardling (PaulGMardling)
left a comment
Paul Mardling (PaulGMardling)
left a comment
There was a problem hiding this comment.
I've left a couple comments on edge cases.
Let me know if you think they're relevant, or if they are too specific and out of scope
| text | ||
| .replace(BIDI_MARKS, '') | ||
| .replace(NO_BREAK_SPACES, ' ') | ||
| .replace(/./g, char => digits.get(char) ?? char); |
There was a problem hiding this comment.
Could we use Unicode-aware matching (/./gu) when normalizing digits? In an ff-Adlm runtime, our formatter produces 𞥑𞥔:𞥓𞥐:𞥔𞥕 for 14:30:45, but parsing it returns invalid-input: /./g splits each digit’s surrogate pair before looking it up in the map. That means users cannot commit the localized time we display. Could we add an Adlam round-trip test too?
| case 'dayPeriod': | ||
| return `(?<dayPeriod>${dayPeriods})`; | ||
| case 'literal': | ||
| return value.trim() ? `(?::|${escapeRegExp(value)})` : value; |
There was a problem hiding this comment.
Could we distinguish separators between fields from trailing unit labels? With fr-CA and seconds enabled, 14:30:45 is rejected, but 14:30:45: succeeds because the trailing " s" becomes a required (?::| s) match. Could we accept the documented colon-separated form without requiring a suffix, retain support for 14 h 30 min 45 s, and cover both forms in a regression test?
This branch has not been deployed
3 participants
Stack: #36811 scaffold (merged) → #36845 spec → #36812 time utilities → #36813 TimePicker → #36814 headless TimePicker
New Behavior
Ports the time math from react-timepicker-compat into
src/utilstogether with its tests: option generation, start/end anchors, formatting, and parsing/validation of time strings.Types that were internal in compat (
Hour,TimeFormatOptions,TimePickerOption,TimeStringValidationResult,TimePickerErrorType) move toutils/types.ts. They become public together with the component in #36813