feat(TimeRangeField): Implement TimeRangeField Component#2206
Merged
Conversation
commit: |
epr3
force-pushed
the
feat-2182-time-range-field
branch
2 times, most recently
from
56f5cdb to
777084c
Compare
epr3
force-pushed
the
feat-2182-time-range-field
branch
from
777084c to
cdf8ffe
Compare
epr3
force-pushed
the
feat-2182-time-range-field
branch
from
cdf8ffe to
cf88efc
Compare
Member
|
@epr3 will merge this to the next minor release, alongside with another possible component 🤞🏻 |
Collaborator
Author
|
Sounds good! 👍 |
Contributor
|
This would be a really cool feature to publish @zernonia ;) |
Contributor
|
@epr3 can you please update code to update the pkg-pr-new package |
epr3
force-pushed
the
feat-2182-time-range-field
branch
from
cf88efc to
6e7c671
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a TimeRangeField feature: new Root and Input components with context API, locale-aware formatting, granularity, keyboard navigation, validation, types/exports, tests, stories, demos, docs, and site navigation entry. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Input as TimeRangeFieldInput
participant Root as TimeRangeFieldRoot
participant Formatter as DateFormatter
participant Context as RootContext
User->>Input: focus / click segment
Input->>Root: request focus / emit interaction
Root->>Context: update focused element / segment state
Root->>Formatter: format segment value (locale, hourCycle)
Formatter-->>Root: formatted segment parts
Root->>Input: provide segment display & attributes
Input-->>User: render segment (aria, value)
User->>Input: type value / press Arrow
Input->>Root: emit update:modelValue or navigation event
Root->>Root: validate (min/max/unavailable, order)
Root-->>Input: update segment state or move focus
Input-->>User: update UI and focus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@docs/components/demo/TimeRangeField/css/styles.css`:
- Around line 17-28: Remove the redundant border-width declaration in the
.TimeField rule: the existing border-width: 1px; is immediately overridden by
the border: 1px solid var(--gray-9); shorthand, so delete the border-width line
from the .TimeField CSS (locate the .TimeField selector in styles.css).
- Around line 54-55: The selector `.TimeFieldSegment:[aria-valuetext='Empty']`
is invalid CSS; remove the extraneous colon so the class and attribute selector
are combined (i.e., place the attribute selector immediately after
`.TimeFieldSegment`) to target elements with class TimeFieldSegment that have
aria-valuetext="Empty". Update the rule for `.TimeFieldSegment` +
`[aria-valuetext='Empty']` accordingly so the color declaration applies
correctly.
In `@docs/components/demo/TimeRangeField/tailwind/index.vue`:
- Around line 20-22: The v-for loops over segments.start and segments.end use
:key="item.part" which can collide for multiple 'literal' parts; change both
loops (the one iterating over segments.start and the one over segments.end) to
include the loop index (e.g., (item, idx) in segments.start / segments.end) and
build a stable unique key combining item.part and the index (such as
`${item.part}-${idx}`) so each rendered fragment has a unique key and Vue
warnings are eliminated.
In `@docs/content/docs/components/time-range-field.md`:
- Around line 675-700: The example uses Vue's watch but doesn't import it;
update the <script setup> imports to include watch (alongside ref) so the
watcher on timeRange works—locate the import line that brings in ref (and
possibly Time) and add watch to that import list to ensure the watch(timeRange,
...) call is defined.
- Around line 161-169: The examples use the Time constructor but do not import
it; add an import for Time from 'reka-ui' alongside TimeRangeFieldInput and
TimeRangeFieldRoot in the <script setup> blocks (e.g., import { Time,
TimeRangeFieldInput, TimeRangeFieldRoot } from 'reka-ui') so new Time(9, 0) and
other occurrences resolve; apply the same import addition to all subsequent
examples in this file that call new Time(...).
In `@docs/content/meta/TimeRangeFieldInput.md`:
- Around line 18-21: Update the 'part' field's description to refer to "time"
instead of "date" in the TimeRangeFieldInput metadata: change the description
string for 'part' (currently "<p>The part of the date to render</p>\n") to use
"time" ("<p>The part of the time to render</p>\n"); if this file is generated,
update the source template that produces the 'part' description so future
regenerations use "time" not "date".
In `@docs/content/meta/TimeRangeFieldRoot.md`:
- Around line 18-105: The docs for TimeRangeFieldRoot contain date/calendar
wording and a redundant granularity sentence; update the generator templates
(source templates/generator) that produce the descriptions for properties like
defaultPlaceholder, defaultValue, granularity, placeholder (and any other
TimeValue/TimeRange fields) to use time-specific wording (e.g., "time" instead
of "date/calendar") and rewrite the granularity description to a single clear
sentence explaining that it controls the displayed time segments up to the
specified unit (hour|minute|second) and the default behavior; ensure the
generator emits the corrected descriptions for 'defaultPlaceholder',
'defaultValue', 'granularity', and 'placeholder'.
In `@packages/core/src/DateRangeField/DateRangeFieldRoot.vue`:
- Around line 114-123: Update the JSDoc in the defineSlots call for
DateRangeFieldRoot to replace references to "time" with "date" or "date range"
so the slot description correctly reflects the component; specifically update
the comment for modelValue (currently "The current time of the field") to "The
current date or date range of the field" and adjust the segments comment
(currently "The time field segment contents") to "The date field segment
contents" while keeping the existing props names (modelValue, segments,
isInvalid) and types unchanged.
In `@packages/core/src/TimeRangeField/story/_DummyTimeRangeField.vue`:
- Around line 17-20: The v-for over segments.start uses item.part as the key
which can repeat (e.g., multiple "literal" segments); update the keying in both
the start and end loops (the template iterating segments.start and the
corresponding template for segments.end around lines 38-41) to use a stable
unique key such as the loop index or a composite key combining item.part with
the index (e.g., `${item.part}-${index}`) to avoid duplicate keys and render
instability.
In `@packages/core/src/TimeRangeField/story/_TimeRangeField.vue`:
- Around line 42-46: Remove the invalid disabled attribute from the <span>
element (the element with data-testid="value" that renders {{ modelValue }});
keep tabindex="-1" to prevent focus and, if you need to convey
non-interactive/disabled semantics, add aria-disabled="true" to that same span
instead of disabled.
- Around line 23-27: The v-for uses non-unique keys (item.part) in
TimeRangeFieldInput which can repeat (e.g., multiple 'literal' segments); update
the v-for to include the iteration index (e.g., v-for="(item, idx) in
segments.start") and change the :key to a composite value using the part and
index (e.g., `${item.part}-${idx}`) to guarantee uniqueness; apply the same
change for the corresponding segments.end v-for loop so both TimeRangeFieldInput
lists use unique keys.
In `@packages/core/src/TimeRangeField/story/TimeRangeFieldDefault.story.vue`:
- Around line 13-16: The v-for templates use :key="item.part" which can
duplicate for repeated literal segments; update both v-for blocks (the template
iterating over segments.start and the template iterating over segments.end) to
use a unique key such as the loop index or a composite key (e.g.,
`${item.part}-${index}`) instead of item.part so each rendered segment has a
stable, unique key.
In `@packages/core/src/TimeRangeField/TimeRangeField.test.ts`:
- Line 49: The describe block for 'timeField' is incorrectly declared async;
remove the async modifier from the describe callback (the describe('timeField',
async () => { ... }) declaration) so that only the individual it/test callbacks
handle async operations—update the declaration to describe('timeField', () => {
... }) and ensure no awaits remain in that describe body.
- Around line 70-82: The test for ZonedDateTime is flaky due to locale-dependent
formatting; update the setup call in this spec to pin the locale and hour cycle
by passing locale: 'en-US' and hourCycle: 'h12' in the props (e.g., include
locale: 'en-US' and hourCycle: 'h12' alongside timeRangeFieldProps: {
modelValue: zonedDateTime }) so the AM/PM and EST expectations (and the 12-hour
end.hour calculation) are stable across environments.
In `@packages/core/src/TimeRangeField/TimeRangeFieldRoot.vue`:
- Around line 419-437: The hourCycle is passed into
provideTimeRangeFieldRootContext as a static value (props.hourCycle) and won't
react to parent updates; change the argument to pass a reactive ref/computed
instead (e.g., computed(() => props.hourCycle) or ref(props.hourCycle)) or
update the context type to accept Ref<HourCycle|undefined> and provide
props.hourCycle as a ref so children consuming hourCycle from
provideTimeRangeFieldRootContext will update when the prop changes.
In `@time-range-field.md`:
- Around line 304-318: The helper function isTimeRangeValid is defined but never
used; either remove the unused function or wire it into the example validation
flow by invoking isTimeRangeValid wherever time-range checks occur (e.g., in the
validation handler or function that validates timeRange objects) and ensure it
relies on isTimeUnavailable for business-hours checks; update any callers or
docs to reference isTimeRangeValid (or delete the function and remove any
related references to avoid dead code).
🧹 Nitpick comments (5)
packages/core/src/TimeRangeField/story/_TimeRangeField.vue (1)
2-7: AlignonUpdate:modelValuetype with range value
update:modelValuefor a range should use the range type, not a singleTimeValue. This keeps story/test typing aligned with the component API.✅ Suggested fix
-import type { TimeValue } from '@/shared/date' ... -const props = defineProps<{ timeRangeFieldProps?: TimeRangeFieldRootProps, emits?: { 'onUpdate:modelValue'?: (data: TimeValue) => void } }>() +const props = defineProps<{ timeRangeFieldProps?: TimeRangeFieldRootProps, emits?: { 'onUpdate:modelValue'?: (data: TimeRangeFieldRootProps['modelValue']) => void } }>()packages/core/src/TimeRangeField/story/TimeRangeFieldChromatic.story.vue (1)
53-55: Timezone example variant doesn't demonstrate timezone functionality.This variant is identical to the "Placeholder" variant and doesn't actually showcase timezone handling. Consider either removing it or adding a
ZonedDateTimevalue to properly demonstrate timezone behavior.packages/core/src/TimeRangeField/TimeRangeFieldRoot.vue (3)
463-473: VisuallyHidden input value may display "undefined - undefined".When both
startandendare undefined, the hidden input's value will be the string"undefined - undefined". Consider providing a fallback empty string for better form handling.Proposed fix
<VisuallyHidden :id="id" as="input" feature="focusable" tabindex="-1" - :value="`${modelValue?.start?.toString()} - ${modelValue?.end?.toString()}`" + :value="modelValue?.start && modelValue?.end ? `${modelValue.start.toString()} - ${modelValue.end.toString()}` : ''" :name="name" :disabled="disabled" :required="required"
234-242: Computed setters return values unnecessarily.The
return newValuestatements in the computed setters forconvertedStartValue,convertedEndValue, andconvertedPlaceholderare unnecessary. Vue computed setters don't use return values.Proposed fix for convertedStartValue (apply same pattern to others)
set(newValue) { if (newValue) { startValue.value = startValue.value && 'day' in startValue.value ? newValue : new Time(newValue.hour, newValue.minute, newValue.second, startValue.value?.millisecond) } else { startValue.value = newValue } - return newValue },Also applies to: 251-259, 278-282
326-340: Duplicate watchers onconvertedModelValue.There are two separate
watchcalls onconvertedModelValue(lines 326-340 and 364-367). These could be consolidated into a single watcher for clarity and to avoid potential ordering issues.Proposed consolidation
watch(convertedModelValue, (_modelValue) => { const isStartChanged = _modelValue?.start && convertedStartValue.value ? _modelValue.start.compare(convertedStartValue.value) !== 0 : _modelValue?.start !== convertedStartValue.value if (isStartChanged) { convertedStartValue.value = _modelValue?.start?.copy() } const isEndChanged = _modelValue?.end && convertedEndValue.value ? _modelValue.end.compare(convertedEndValue.value) !== 0 : _modelValue?.end !== convertedEndValue.value if (isEndChanged) { convertedEndValue.value = _modelValue?.end?.copy() } + + // Sync placeholder with start value + if (_modelValue && _modelValue.start !== undefined && placeholder.value.compare(_modelValue.start) !== 0) + placeholder.value = _modelValue.start.copy() }) - -watch(convertedModelValue, (_modelValue) => { - if (_modelValue && _modelValue.start !== undefined && placeholder.value.compare(_modelValue.start) !== 0) - placeholder.value = _modelValue.start.copy() -})Also applies to: 364-367
5 tasks
epr3
force-pushed
the
feat-2182-time-range-field
branch
from
6e7c671 to
03775c4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/components/demo/TimeRangeField/css/styles.css`:
- Line 24: Replace the long hex color in the background-color declaration in
styles.css (the background-color: `#ffffff`; line) with the shorthand form `#fff` to
satisfy the color-hex-length lint rule; update the background-color property
value to use `#fff` wherever that exact declaration appears in TimeRangeField's
styles.
In `@packages/core/src/TimeRangeField/TimeRangeFieldRoot.vue`:
- Around line 148-150: The locale-change watcher is calling getSegmentElements
(which selects [data-reka-date-field-segment]) but the mounted code and
TimeRangeField expect time-field segments; update the watcher to call
getTimeFieldSegmentElements instead so it repopulates segmentElements with the
correct [data-reka-time-field-segment] nodes. Locate the locale watcher that
currently uses getSegmentElements and replace that call with
getTimeFieldSegmentElements, ensuring it still iterates and adds items into the
segmentElements Set the same way as the onMounted block.
In `@time-range-field.md`:
- Line 172: The v-for in the template iterates over segments.start using
:key="item.part" which can produce duplicate keys (e.g., repeated "literal"
parts); update the template's v-for to capture the index (template v-for="(item,
index) in segments.start") and make the key unique by combining part and index
(use a template string like `${item.part}-${index}`) so each rendered segment
(in the template with v-for, segments.start, and item.part) has a stable unique
key.
- Line 62: The prop description in TimeRangeField for the granularity prop is
redundant (it currently says "Defaults to minute if a Time is provided,
otherwise defaults to minute"); update the granularity description in
time-range-field.md (the TimeRangeField prop docs) to a single clear default
statement such as "Defaults to minute." so it no longer repeats the same branch;
ensure the prop name "granularity" and the component name "TimeRangeField"
remain referenced correctly in the updated text.
🧹 Nitpick comments (3)
packages/core/src/TimeRangeField/TimeRangeFieldRoot.vue (3)
326-340: Two separate watchers onconvertedModelValue— consider merging.There are two
watch(convertedModelValue, ...)callbacks (lines 326 and 364). This splits related synchronization logic across two watchers on the same source, making the execution order harder to reason about. Consider merging them into a single watcher.Also applies to: 364-367
292-292:initialSegmentsis not reactive to granularity changes.
initialSegmentsis computed once frominferredGranularity.valueat initialization time. Ifgranularityprop changes dynamically, the reset logic in the watchers (lines 348 and 375) will use stale segment shapes. Consider making it acomputed.Suggested fix
-const initialSegments = initializeTimeSegmentValues(inferredGranularity.value) +const initialSegments = computed(() => initializeTimeSegmentValues(inferredGranularity.value))Then update usages to
initialSegments.value:- startSegmentValues.value = { ...initialSegments } + startSegmentValues.value = { ...initialSegments.value }
136-138: Formatter created with non-reactive locale.
useDateFormatter(locale.value, ...)receives the unwrapped string, so the formatter's internalhourCycleoption is fixed at creation time. While the locale watcher (line 352) manually callsformatter.setLocale(value), thehourCyclenormalization passed during construction won't be refreshed ifhourCycleprop changes. This is consistent with the non-reactivehourCycleissue already flagged but worth noting the formatter is also affected.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Closes #2182
Summary by CodeRabbit
New Features
Documentation
Tests
Stories