feat(base-ui): date/time/range pickers with sectioned guided input#38
Open
joshuapare wants to merge 41 commits into
Open
feat(base-ui): date/time/range pickers with sectioned guided input#38joshuapare wants to merge 41 commits into
joshuapare wants to merge 41 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive suite of six date/time picker components to the base-ui package, including DateField, DatePicker, Calendar, DateRangePicker, DateTimePicker, and TimePicker. It includes helper hooks, utility functions, CSS styling modules, extensive test coverage, and documentation updates, along with build configuration changes for the showcase app. Changes
Sequence DiagramsequenceDiagram
actor User
participant DateField
participant useDateField
participant sections as sections.ts
participant DOM
User->>DateField: Click/Type input
DateField->>useDateField: Pass user interaction
useDateField->>sections: applyDigitToSection()<br/>applyPaste()<br/>validateSections()
sections-->>useDateField: Updated value +<br/>validation result
useDateField->>useDateField: Sync state &<br/>compute validity
useDateField-->>DOM: Update sections &<br/>emit onChange()
DOM-->>User: Reflect changes
sequenceDiagram
actor User
participant DatePicker
participant Popover
participant Calendar
participant dateUtils as dateUtils.ts
participant DOM
User->>DatePicker: Click calendar icon
DatePicker->>Popover: Open popover
Popover->>Calendar: Render month grid
Calendar->>dateUtils: getMonthMatrix()<br/>isSameDay()<br/>isBefore()/isAfter()
dateUtils-->>Calendar: Grid matrix &<br/>date comparisons
Calendar->>DOM: Render month + cells
User->>Calendar: Click day or navigate
Calendar->>dateUtils: clampDate()<br/>addMonths()/addYears()
dateUtils-->>Calendar: Adjusted dates
Calendar-->>DatePicker: Emit onChange()
DatePicker->>Popover: Close (if done)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Storybook Preview Updated: c99f8ec |
Zero-dependency date helpers (startOfDay, startOfMonth, addMonths, addDays, addYears, isSameDay, isBefore, isAfter, clampDate, isDateInRange, getMonthMatrix) with 10 exhaustive unit tests covering calendar correctness including month-end clamping and week alignment.
Wraps Calendar in a Base UI Popover with controlled/uncontrolled value support, Intl-based formatting, and full keyboard accessibility (Escape returns focus to trigger, day selection closes popup).
Adds stories for DatePicker (Default, Controlled, WithMinMax, LongFormat, Disabled), TimePicker (TwentyFourHour, TwelveHour, WithSeconds, MinuteStep15, Disabled), and DateTimePicker (Default, WithSeconds, TwelveHour), following project conventions (satisfies Meta, autodocs, render + args pattern).
Add Calendar, DatePicker, DateTimePicker, and TimePicker to the approved component set in COMPONENT_STATUS.md with brief feature descriptions.
Adds a live showcase demo for DatePicker, TimePicker, and DateTimePicker — each displaying its current value alongside the picker, with meaningful variations (min/max bounds, 12-hour + seconds, 24-hour + minuteStep=15).
Switch DatePicker and DateTimePicker from raw @base-ui/react/popover to the project's Popover wrapper, giving the portal and positioner proper z-index and the popup a solid background with border/shadow so no bleed-through occurs. Redesign all three picker triggers to look like form inputs (matching Select/Input shell style): inline-flex with full border, control-height, space-inline-control padding, placeholder-tinted value text on the left, and a contextual icon (LuCalendar / LuClock / LuCalendarClock from react-icons/lu) pinned to the right. TimePicker root is now an input-shell wrapper; when embedded inside DateTimePicker's popup the shell styling is stripped via a scoped CSS override in DateTimePicker.module.css.
Replace the button trigger with a controlled text input that accepts direct typing; parse/validate on blur or Enter, revert on Escape, and commit calendar selections immediately. A right-side icon button opens the existing calendar popover, anchored to the full input shell.
Adds a scrollable column-picker popover (hours, minutes, optional seconds, optional AM/PM) triggered by the clock icon button, while preserving all existing text-field typing behavior. Also adds 5 new tests covering popover open/close, column selection, minuteStep, and AM/PM switching.
Add six Storybook stories for DateRangePicker (Default, Controlled, WithMinMax, Disabled, CustomSeparator, LongFormat) following the DatePicker stories conventions. Create COMPONENT_STATUS.md documenting all five picker components with updated descriptions for DatePicker (text input + live parse) and TimePicker (text entry + column-selector).
Adds a headless + component pair for guided, validated, keyboard-navigable date/time entry split into individually-editable sections (MM/DD/YYYY/HH/mm/ss/AM-PM). - sections.ts: pure helpers — buildSections, validateSections, getDaysInMonth, adjustSectionValue, applyDigitToSection, applyPaste, clampDayToMonth, etc. Uses Intl.DateTimeFormat.formatToParts for locale-aware tokenization; no date-lib dependency. - useDateField: headless hook managing section state, focus, keyboard (Tab, Arrow Up/Down, digit input with auto-advance, Backspace, Escape), paste. - DateField: component wrapper rendering per-section contentEditable spans + literal separators. CSS Modules with tabular-nums, data-focused/placeholder hooks, existing OV tokens only. - 85 new tests (47 pure helpers + 38 component), all passing. This is the foundation for DatePicker/TimePicker/DateTimePicker/DateRangePicker sectioned input mode; wiring into those components is a separate task.
Replace the plain <input type="text"> trigger with the new DateField primitive; remove all draft/parse state in favour of DateField's own section editing, and add rangeError state to flag min/max/isDateDisabled violations without calling external onChange.
Replace inline hour/minute/second text inputs and AM/PM toggle button with a single DateField in time mode; column-selector popover is preserved intact.
Replace the plain button trigger with a shell + DateField mode="datetime" + calendar-clock icon button, matching the DatePicker pattern. Add shell/error/disabled/iconButton CSS, shellRef-anchored popover, handleFieldChange with range validation, and rangeError state. Expand tests from 3 to 5.
Replace the single combobox text-input with two DateField primitives (start + end) joined by a static separator span and a single calendar icon button, mirroring the DatePicker shell pattern.
…d inputs Add DateField.stories.tsx with 7 stories covering all modes, locales, and accessibility states. Update COMPONENT_STATUS.md to reflect the sectioned DateField primitive and its use as the trigger for all picker components. Also make DateField.value optional (default null) to align with DatePicker API.
…s row layout
- Add `bare` prop to DateField that disables its input-shell styling
(border, background, padding, min-height, hover/focus-within ring) via
a `data-bare` attribute and matching CSS selectors. Standalone DateField
usage is unchanged; pickers pass `bare` to avoid the doubled border.
- Pass `bare` to the embedded DateField in DatePicker, TimePicker,
DateTimePicker, and both DateField instances in DateRangePicker.
- Add `.shell > [role="group"] { flex: 1; min-width: 0 }` in each picker's
CSS so the bare DateField grows to fill space between leading side and
the trailing icon button.
- Change DateTimePicker popup from column to row layout: Calendar on the
left, time panel on the right separated by a vertical border. The right
panel has a "Time" label, min-width 160px, and column flex-direction.
Extract TimeColumn + column builders into a standalone TimeColumns component, then wire DateTimePicker's popup to use it directly instead of embedding a full TimePicker with its own input shell, label, and clock-icon popover trigger.
joshuapare
force-pushed
the
feat/date-time-pickers
branch
from
450ebe0 to
42cf5ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/COMPONENT_STATUS.md (1)
23-75:⚠️ Potential issue | 🟡 MinorKeep the approved component list in sync with the new exports.
This list now documents the new picker family but omits
DateFieldandDateRangePicker, both of which are exposed via new public barrels in this PR. Since Line 7 says these are the only exported in-scope components, the doc becomes stale immediately.📝 Proposed doc update
- `ContextMenu` +- `DateField` — sectioned guided date/time input with per-section editing and validation - `DatePicker` — calendar popover input with full keyboard navigation, min/max constraints, and custom disabled-cell callback +- `DateRangePicker` — range calendar popover input for selecting start and end dates - `DateTimePicker` — composition of `DatePicker` + `TimePicker` producing a combined date-and-time value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/COMPONENT_STATUS.md` around lines 23 - 75, Update the approved component list to include the newly exported picker components by adding `DateField` and `DateRangePicker` to the bulleted list alongside `DatePicker` and `TimePicker`, ensuring the documented set of exported components (e.g., `DatePicker`, `DateTimePicker`, `TimePicker`, `DateField`, `DateRangePicker`) matches the new public barrels introduced in this PR and keeping the sentence that asserts "these are the only exported in-scope components" accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase/package.json`:
- Line 8: The npm script key "build:deps" uses a pnpm filter selector with
single quotes and the ^ character which is not recognized on Windows; update the
script value for "build:deps" to use double quotes around the filter selector
(replace '@omniviewdev/showcase^...' with "@omniviewdev/showcase^...") so the
pnpm filter works cross-platform in cmd.exe and PowerShell.
In `@docs/superpowers/plans/2026-04-12-date-time-pickers.md`:
- Around line 9-33: Replace the hard-coded author-local absolute path
"/Users/joshuapare/Repos/omniviewdev/omniview-ui" in
docs/superpowers/plans/2026-04-12-date-time-pickers.md with a repo-relative
placeholder (e.g. use the variable "$REPO_ROOT") and update the Working
Directory & Worktree command sequence so the first cd uses cd "$REPO_ROOT"
before running git worktree add and the subsequent steps; ensure the example
shows the repo-relative flow around the existing git worktree add, pnpm install,
pnpm --filter `@omniviewdev/base-ui` test, and pnpm --filter `@omniviewdev/base-ui`
build commands.
- Around line 104-130: The markdown has fenced CSS blocks missing a preceding
blank line (causing MD031); add a blank line before each opening ```css fence in
the sections for the theme block introduced by "For
`:root[data-ov-theme='dark']`" and the two density blocks introduced by "For
`:root[data-ov-density='comfortable']`" and "For
`:root[data-ov-density='compact']`" so each fenced block is separated from the
preceding paragraph; ensure there is a blank line before each opening fence (and
keep the existing closing fences intact).
In `@docs/superpowers/plans/2026-04-12-theme-extensibility-system.md`:
- Around line 1718-1735: The markdown has fenced TypeScript blocks nested inside
a numbered list without blank lines, which triggers MD031; add a blank line
before and after each fenced code block shown (the block importing themeRegistry
and SOLARIZED_DARK and the block showing the theme global), ensuring the list
items separate from the triple-backtick fences; verify the sections referencing
themeRegistry, SOLARIZED_DARK, globalTypes.theme.toolbar.items,
themeRegistry.list(), and themeRegistry.apply(context.globals.theme) keep their
surrounding blank lines so the fences are treated as code blocks and
markdownlint stops flagging MD031.
In `@packages/base-ui/docs/COMPONENT_STATUS.md`:
- Around line 7-12: The table in COMPONENT_STATUS.md marks Calendar, DateField,
DatePicker, DateRangePicker, DateTimePicker, and TimePicker as "Stable" despite
outstanding manual UX verifications; update their status to "Beta" (or
"Experimental" per project convention) in the table rows for those exact
component names (Calendar, DateField, DatePicker, DateRangePicker,
DateTimePicker, TimePicker) and add a short note in the status column or a
footnote indicating that final UX verifications are pending and the status will
be promoted once checks complete.
In `@packages/base-ui/src/components/date-field/DateField.stories.tsx`:
- Around line 27-83: The stories call useState inside their render callbacks
which violates react-hooks rules; extract each story's stateful logic into a
separate PascalCase component (e.g., DateModeStory, TimeMode24hStory,
TimeMode12hStory, DateTimeModeStory, LocaleGBStory, DisabledStory,
ReadOnlyStory) that uses useState and returns the DateField JSX, then change
each Story's render to simply return the corresponding component (e.g., render:
(args) => <DateModeStory {...args} />) so hooks are used at the top level of a
component instead of inside the render callback.
In `@packages/base-ui/src/components/date-field/index.ts`:
- Around line 9-15: The barrel currently re-exports Section, SectionType,
buildSections and validateSections but omits the companion option/result types;
update the index to also re-export BuildSectionsOptions (the options type used
by buildSections) and the validate result type (e.g., ValidateSectionsResult or
whatever validateSections exports) so consumers can import types from the same
entrypoint; locate the corresponding type names in sections.ts and add them to
the existing export list alongside Section and SectionType and the functions
buildSections/validateSections.
In `@packages/base-ui/src/components/date-field/sections.test.ts`:
- Around line 543-557: The test's loop uses a variable last typed as number |
null and relies on getNextEditableIndex(sections, last) accepting null; make the
intent explicit by obtaining the first editable index into a const (e.g., const
first = getNextEditableIndex(sections, null)), assert it is not null (throw or
expect toBeDefined), then seed last as that non-null number before the while
loop so subsequent calls to getNextEditableIndex(sections, last) use a narrowed
number type; alternatively, if you prefer minimal change, cast when reassigning
(e.g., last = next as number) to show the value is expected to be a number.
Ensure references: getNextEditableIndex, sections, last, next.
- Around line 516-524: The test description claims the year should still be set
but the test never asserts it; update the test that calls buildSections and
applyPaste (the case with mode: 'date', locale: 'en-US', value: null) to also
find the section with type 'year' and assert its value equals '2026' so the
intended behavior of applyPaste (skipping out-of-range month/day but consuming
the year token) is verified.
In `@packages/base-ui/src/components/date-field/sections.ts`:
- Around line 363-368: validateSections currently infers 12-hour mode by
checking the hour Section's s.max === 12 which couples validation to how
Sections are constructed; instead, propagate the hour-cycle explicitly into
validateSections (or add a discriminator on the hour Section) so it doesn't
re-infer from numeric bounds: update buildSections (the producer of Sections) to
stamp an explicit flag (e.g., is12Hour or hourCycle) onto the hour Section, then
change validateSections to read that flag (or accept an hourCycle parameter) and
use it when setting has12HourHour / meridiem logic rather than testing s.max;
reference symbols: buildSections, validateSections, s.max, has12HourHour, hour
Section, meridiem.
- Around line 644-655: The comment and code disagree for the meridiem branch:
currently both the if and else increment tokenIdx, which consumes stray digit
tokens instead of letting them be matched by subsequent digit sections; update
the else branch in the section.type === 'meridiem' handling so it does not
advance tokenIdx (leave the section unchanged and simply continue), or if the
original intent was to consume non-meridiem tokens, update the comment to
reflect that; specifically edit the meridiem block around tokenIdx, sectionIdx,
token and letters so the else no longer does tokenIdx++ (or adjust the comment
to match the existing tokenIdx++ behavior).
- Around line 687-693: rebuildSectionsFromValue is a redundant wrapper around
buildSections; remove the extra public symbol or implement meaningful rebuild
behavior. Either (A) delete rebuildSectionsFromValue and update all call sites
to call buildSections(options) directly, or (B) implement actual rebuild logic
inside rebuildSectionsFromValue (for example preserve partial user edits by
merging existing Section[] state with newly built sections) so its behavior
differs from buildSections; reference the functions rebuildSectionsFromValue and
buildSections when making changes.
- Around line 65-84: The conditional guards in getDateFormatOptions and
getTimeFormatOptions are dead because their parameter unions already restrict
mode (getDateFormatOptions: mode: 'date' | 'datetime'; getTimeFormatOptions:
mode: 'time' | 'datetime'); remove the unreachable branches and simplify each
function to always build and return the appropriate Intl.DateTimeFormatOptions
for the declared mode (i.e., delete the ternary that returns {} in
getDateFormatOptions and remove the early-return guard in getTimeFormatOptions),
keeping the existing option construction logic and the showSeconds/hourCycle
handling.
In `@packages/base-ui/src/components/date-field/useDateField.ts`:
- Around line 363-372: handleRootPaste prevents default and updates sections via
setSections(applyPaste(...)) but does not move focus after a multi-section
paste; update handleRootPaste so after computing the new sections (call
applyPaste with the previous sections and text) you determine the index of
either the first editable empty section or the index immediately after the last
filled section and then programmatically select/focus that section (use the
component's existing section-focus helper such as setSelectedSection or
focusSection if present). Ensure you derive the next-focus index from the
applyPaste result (not the old prev), and perform the focus change after
setSections (e.g., in the same callback or via a microtask) so the UI advances
to the expected section.
- Around line 283-306: The code currently defers focus movement with
queueMicrotask in the digit branch (after
applyDigitToSection/setSections/clampDayToMonth) and the meridiem branch (after
setSectionValue), which can capture a stale focusedIndex; replace those
queueMicrotask calls by calling focusNext(focusedIndex) synchronously after
state update, or if you must ensure the state is committed before moving focus
wrap the setSections call with flushSync from react-dom and then call focusNext
immediately; update both usages (the queueMicrotask invocations near
applyDigitToSection/setSectionValue and the meridiem branch) to use the
synchronous approach and remove queueMicrotask.
In `@packages/base-ui/src/components/date-picker/Calendar.tsx`:
- Around line 58-63: The date-range logic compares raw timestamps which treats
same calendar days at different times as different; update isStrictlyBetween and
the other comparison sites (the functions using getTime() at the other mentioned
diffs) to compare dates at start-of-day by normalizing inputs (e.g.,
truncateTime(date) that returns a new Date with hours/minutes/seconds/millis set
to 0 or use date.setHours(0,0,0,0) on copies) and then compare their times, so
day-grid clicks and startDate on the same calendar day are considered equal for
range logic; locate and replace uses of d.getTime(), a.getTime(), b.getTime() in
isStrictlyBetween and the two other comparison helpers with
normalizedDate.getTime() calls.
- Around line 230-248: When Prev/Next header buttons call setViewMonth (in the
Calendar component), also update the roving-focus state so focusedDate remains
inside the new 6×7 matrix; compute newMonth = addMonths(viewMonth, ±1) and if
the current focusedDate is not in the generated matrix for newMonth,
setFocusedDate to a valid date in that matrix (e.g., clamp the day number into
the new month or set to the first visible day of the matrix); update both
setViewMonth(...) and setFocusedDate(...) together so a tabIndex=0 day cell
always exists after month navigation.
In `@packages/base-ui/src/components/date-picker/DatePicker.stories.tsx`:
- Around line 20-76: The story render callbacks (Default, Controlled,
WithMinMax, LocaleGB, Disabled) use useState inside arrow functions which breaks
the hooks rules; extract each render into a named PascalCase React component
(e.g., DefaultStory, ControlledStory, WithMinMaxStory, LocaleGBStory,
DisabledStory) that calls useState and returns the DatePicker with the
appropriate props (value, onChange, min, max, locale, disabled), then update
each Story's render to return the corresponding component (e.g., render: () =>
<DefaultStory />) so hooks are only used inside proper React components and
DatePicker usage remains unchanged.
In `@packages/base-ui/src/components/date-picker/DatePicker.test.tsx`:
- Around line 62-78: The test renders DatePicker with value={null} and never
triggers an out-of-range value, so it doesn't exercise the "shellError" path;
update the test for DatePicker to actually provide a value outside the min/max
(e.g., render with value={new Date(2026,3,10)} when min is 2026-04-15 or
rerender after initial render with a value outside the provided min/max) or
simulate a DateField change via the onChange handler to set an out-of-range
date, then assert screen.getByTestId('date-picker-shell').className matches
/shellError/ to verify the error state is applied.
In `@packages/base-ui/src/components/date-picker/DatePicker.tsx`:
- Around line 139-148: The trigger button visually and accessibly ignores the
component's readOnly prop: although handleIconButtonClick early-returns when
readOnly, the <button> only uses disabled={disabled} so it looks interactive and
lacks aria-disabled for AT users; update the button rendering (the element that
renders LuCalendar with className={styles.iconButton}) to reflect readOnly by
either setting disabled={disabled || readOnly} for parity with the disabled path
or, if you want the button to remain focusable, add aria-disabled={readOnly ||
undefined} (and keep disabled={disabled}) so the control both looks and
announces its non-interactive state consistently.
- Around line 48-52: Replace the local isDateInRange helper in DatePicker.tsx
with the shared isDateInRange from ./dateUtils: remove the duplicate function
definition and add an import for isDateInRange (the same helper already used by
DateRangePicker) so DatePicker's range checks use the start-of-day semantics
(and avoid raw Date < / > comparisons), ensuring consistent behavior across
DatePicker, DateRangePicker, and DateField when times are present.
In `@packages/base-ui/src/components/date-picker/dateUtils.ts`:
- Around line 28-32: addYears currently uses setFullYear which on Feb 29 rolls
into March for non-leap target years; mirror the clamp logic used in addMonths
to prevent month overflow by clamping to the last day of the target month. In
the addYears function, compute the target year (out.getFullYear() + n), get the
last day of the target month for the existing month (e.g., by creating a
Date(targetYear, month + 1, 0) to find month length), and if the original day >
lastDay clamp the day to lastDay before constructing/setting the new date;
update addYears to preserve time components and use the same clamp approach as
addMonths to ensure Feb 29 becomes Feb 28 when the target year is not a leap
year.
In `@packages/base-ui/src/components/date-picker/formatters.test.ts`:
- Around line 35-39: The test for getWeekdayLabels is ambiguous because using
the 'narrow' form and /^S/ could match Saturday; change the assertion to use a
less-ambiguous width ('short' or 'long') and assert the actual ordered labels
(at least the first few) to prove Sunday is first. Locate the test for
getWeekdayLabels in formatters.test.ts, call getWeekdayLabels('en-US', 0,
'short'|'long') and assert the returned array's first elements explicitly (e.g.,
first three labels) to verify weekStartsOn ordering.
In
`@packages/base-ui/src/components/date-range-picker/DateRangePicker.stories.tsx`:
- Around line 22-105: The Storybook stories (Default, Controlled, WithMinMax,
Disabled, CustomSeparator, LocaleGB) currently call useState inside their render
lambdas causing react-hooks/rules-of-hooks violations; fix by extracting each
render's stateful logic into a named PascalCase React component (e.g.,
DefaultStory, ControlledStory, WithMinMaxStory, DisabledStory,
CustomSeparatorStory, LocaleGBStory) that uses useState and returns the
DateRangePicker, then update each Story's render to simply return <DefaultStory
{...args} /> (or the corresponding component) while passing args through; ensure
component names start with uppercase and move any date calculations
(today/min/max) into those components so hooks are only used inside proper React
function components.
In `@packages/base-ui/src/components/date-range-picker/DateRangePicker.tsx`:
- Around line 155-166: The start DateField currently sets max={current.end ??
max}, which prevents typed input from ever triggering the swap/reset branch in
handleStartChange; decide to preserve the typed-input reconciliation: change the
DateField prop to max={max} (remove the current.end cap) so the field accepts
dates after the current end and let handleStartChange (the swap/reset logic
around current.start/current.end) perform the reconciliation; keep the existing
branch in handleStartChange and add a short inline comment near the DateField
and in handleStartChange documenting that the field allows out-of-order typing
and the handler corrects it.
- Around line 83-121: The start/end ordering currently uses raw Date > / <
comparisons which mix time-of-day into ordering; update handleStartChange and
handleEndChange to use the day-level helpers (isAfter and isBefore from
dateUtils) when comparing current.start/current.end with next so ordering
matches isDateInRange/startOfDay semantics; also ensure isAfter and isBefore are
imported (or already exported) from dateUtils and replace the comparisons at the
branches that reset/swap end/start respectively (the checks around "if
(current.end && next > current.end)" and "if (current.start && next <
current.start)").
In `@packages/base-ui/src/components/date-time-picker/DateTimePicker.stories.tsx`:
- Around line 24-45: The story render callbacks (Default, WithSeconds,
TwelveHour) currently call useState inline; extract each into a PascalCase
wrapper component (e.g., DefaultDateTimePicker, WithSecondsDateTimePicker,
TwelveHourDateTimePicker) that manages its own useState<Date | null> and renders
<DateTimePicker {...props} value={value} onChange={setValue} />; then update
each Story.render to return the corresponding wrapper component and keep args
passed through, ensuring you reference the existing story names (Default,
WithSeconds, TwelveHour) and the DateTimePicker component.
In `@packages/base-ui/src/components/date-time-picker/DateTimePicker.test.tsx`:
- Around line 17-33: The DateTimePicker currently closes the combined
calendar/time popover inside its date-change handler; change the onDateChange
(or the handler invoked when a gridcell/day is clicked, e.g., handleDateSelect)
so it only calls the provided onChange with the updated Date (preserving time)
and does NOT call any routine that closes the popover (e.g., remove any
setOpen(false) / closePopover invocation); keep the popover closing behavior
confined to explicit Done/cancel actions or outside-click logic so selecting a
day updates the value but leaves the popover open.
In `@packages/base-ui/src/components/date-time-picker/DateTimePicker.tsx`:
- Around line 100-107: onDateChange currently always calls setOpen(false) which
closes the popover when a date is picked; change it to keep the popover open for
combined date+time mode so users can adjust time in the TimeColumns and use the
Done button. Modify onDateChange to only call setOpen(false) when the component
is in date-only mode (e.g., when a prop like showTime or hasTimeColumns is
false) or when an explicit "closeOnSelect" flag is true; otherwise just update
current with the new date (preserve hours/minutes from current), clear range
error, and let the Done button (or an explicit confirm handler) call
setOpen(false). Reference: onDateChange, current, setCurrent, setOpen, Calendar,
TimeColumns, Done button.
- Around line 184-195: The DateTimePicker passes a fresh Date each render when
current is null (current ?? new Date()), which breaks memoization and causes
drifting defaults; update DateTimePicker to memoize the fallback Date using
React's useMemo (add to the react import) and pass that memoized value to
TimeColumns (e.g., compute const defaultTime = useMemo(() => new Date(), []);
and use value={current ?? defaultTime}) so TimeColumns receives a stable
reference across renders; alternatively move this default logic into TimeColumns
if preferred.
In `@packages/base-ui/src/components/time-picker/TimeColumns.tsx`:
- Around line 107-113: The onKeyDown handler on the li currently calls
onSelect(item.value) directly and skips setting suppressScrollRef.current = true
(unlike the click handler and the ul-level keyboard handler), causing
recentering jitter; update the li onKeyDown branch to set
suppressScrollRef.current = true before calling onSelect when Enter or Space is
pressed (and still preventDefault/stopPropagation), and consider removing one of
the duplicate Enter/Space handlers (either the li onKeyDown or the ul-level
keyboard handler) to avoid divergence; locate and edit the onKeyDown in
TimeColumns.tsx, the click handler reference, the ul keyboard handler, and the
suppressScrollRef usage so behavior matches the click flow and respects the
fullyVisible guard.
In `@packages/base-ui/src/components/time-picker/TimePicker.module.css`:
- Around line 31-43: The hover and focus styles are still applied to disabled
controls; update the CSS selectors so those rules only apply when the control is
not disabled by changing `.root:hover` to `.root:not([data-disabled]):hover` and
`.root:focus-within` to `.root:not([data-disabled]):focus-within` (and apply the
same change to the corresponding rules at 121-132), thereby preventing
hover/focus affordances on elements with the `[data-disabled]` attribute while
keeping the existing `.root[data-disabled]` opacity and cursor styles.
In `@packages/base-ui/src/components/time-picker/TimePicker.stories.tsx`:
- Around line 26-64: Each story (TwentyFourHour, TwelveHour, WithSeconds,
MinuteStep15, Disabled) currently calls useState inside its render callback
which ESLint flags; create a named React component (e.g., DemoTimePicker or
TimePickerDemo) that uses useState<Date> and accepts props to forward to
TimePicker, then update each story's render to return that uppercase component
with {...args} instead of calling useState inline. Ensure the demo component
handles value and onChange state internally and is referenced by each story's
render.
In `@packages/base-ui/src/components/time-picker/TimePicker.test.tsx`:
- Around line 52-68: The test currently computes the snapping math locally
instead of exercising TimePicker's onChange path; update the test to render
TimePicker (with minuteStep=5) and simulate a committed change from the inner
DateField by invoking the DateField's onChange (or simulating user input) with a
Date object having minutes=37, then assert that the TimePicker's onChange mock
was called with a Date whose minutes are snapped to 35; target symbols:
TimePicker, minuteStep, onChange prop, and the DateField's exposed
onChange/commit path (or handleFieldChange) so the component's own snapping
logic is executed and verified.
In `@packages/base-ui/src/components/time-picker/TimePicker.tsx`:
- Around line 10-66: The component currently ignores defaultValue and never
updates internal state, so uncontrolled instances reset to new Date() and Clear
uses today instead of the current value’s date; add an internal state (e.g.,
const [internalValue, setInternalValue] = useState<Date | null>(value ??
defaultValue ?? null)) inside TimePicker, keep it in sync with controlled
updates (useEffect to update internalValue when value prop is not undefined),
and then update internalValue in the change paths: in handleFieldChange call
setInternalValue(snapped or next) before invoking onChange, and in handleClear
compute midnight using the current internalValue’s date (or fallback to new
Date()) and setInternalValue(midnight) before calling onChange; update all
usages that read current to use internalValue (or value when controlled) so both
controlled and uncontrolled modes behave correctly.
---
Outside diff comments:
In `@docs/COMPONENT_STATUS.md`:
- Around line 23-75: Update the approved component list to include the newly
exported picker components by adding `DateField` and `DateRangePicker` to the
bulleted list alongside `DatePicker` and `TimePicker`, ensuring the documented
set of exported components (e.g., `DatePicker`, `DateTimePicker`, `TimePicker`,
`DateField`, `DateRangePicker`) matches the new public barrels introduced in
this PR and keeping the sentence that asserts "these are the only exported
in-scope components" accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c3a4bdc-e92d-4f0d-92c3-6a303a15b543
📒 Files selected for processing (45)
apps/showcase/package.jsondocs/COMPONENT_STATUS.mddocs/superpowers/plans/2026-04-12-date-time-pickers.mddocs/superpowers/plans/2026-04-12-theme-extensibility-system.mdpackages/base-ui/docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/date-field/DateField.module.csspackages/base-ui/src/components/date-field/DateField.stories.tsxpackages/base-ui/src/components/date-field/DateField.test.tsxpackages/base-ui/src/components/date-field/DateField.tsxpackages/base-ui/src/components/date-field/index.tspackages/base-ui/src/components/date-field/sections.test.tspackages/base-ui/src/components/date-field/sections.tspackages/base-ui/src/components/date-field/useDateField.tspackages/base-ui/src/components/date-picker/Calendar.module.csspackages/base-ui/src/components/date-picker/Calendar.test.tsxpackages/base-ui/src/components/date-picker/Calendar.tsxpackages/base-ui/src/components/date-picker/DatePicker.module.csspackages/base-ui/src/components/date-picker/DatePicker.stories.tsxpackages/base-ui/src/components/date-picker/DatePicker.test.tsxpackages/base-ui/src/components/date-picker/DatePicker.tsxpackages/base-ui/src/components/date-picker/dateUtils.test.tspackages/base-ui/src/components/date-picker/dateUtils.tspackages/base-ui/src/components/date-picker/formatters.test.tspackages/base-ui/src/components/date-picker/formatters.tspackages/base-ui/src/components/date-picker/index.tspackages/base-ui/src/components/date-range-picker/DateRangePicker.module.csspackages/base-ui/src/components/date-range-picker/DateRangePicker.stories.tsxpackages/base-ui/src/components/date-range-picker/DateRangePicker.test.tsxpackages/base-ui/src/components/date-range-picker/DateRangePicker.tsxpackages/base-ui/src/components/date-range-picker/index.tspackages/base-ui/src/components/date-time-picker/DateTimePicker.module.csspackages/base-ui/src/components/date-time-picker/DateTimePicker.stories.tsxpackages/base-ui/src/components/date-time-picker/DateTimePicker.test.tsxpackages/base-ui/src/components/date-time-picker/DateTimePicker.tsxpackages/base-ui/src/components/date-time-picker/index.tspackages/base-ui/src/components/editor-tabs/hooks/useTabDetach.test.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/sortable-table/useSortableTable.test.tspackages/base-ui/src/components/time-picker/TimeColumns.tsxpackages/base-ui/src/components/time-picker/TimePicker.module.csspackages/base-ui/src/components/time-picker/TimePicker.stories.tsxpackages/base-ui/src/components/time-picker/TimePicker.test.tsxpackages/base-ui/src/components/time-picker/TimePicker.tsxpackages/base-ui/src/components/time-picker/index.tspackages/base-ui/src/theme/styles.css
Comment on lines
+9
to
+33
| **Tech Stack:** TypeScript 5.9, React 19, Vite 5, Vitest + jsdom, `@testing-library/react`, `@testing-library/user-event`, Storybook 10, `@base-ui/react`, CSS Modules. Source location: `/Users/joshuapare/Repos/omniviewdev/omniview-ui/packages/base-ui/`. | ||
|
|
||
| **Spec:** `docs/superpowers/specs/2026-04-12-library-readiness-for-ide-migration-design.md` | ||
|
|
||
| **Independence:** This plan does not depend on the theme extensibility plan. They can be executed in parallel worktrees. | ||
|
|
||
| **Cross-plan coordination note:** The theme extensibility plan adds a `generate:token-keys` script that parses `styles.css` into typed token unions. When both branches merge, run `pnpm --filter @omniviewdev/base-ui generate:token-keys` and commit the refreshed `src/theme/generated/tokenKeys.ts` — this is what lets custom themes override the new `color.datepicker.*` tokens. If the theme extensibility plan has not yet landed, this step is a no-op; the picker tokens still work because the components reference them via CSS `var()` directly. | ||
|
|
||
| --- | ||
|
|
||
| ## Working Directory & Worktree | ||
|
|
||
| All file paths in this plan are relative to the base-ui package: | ||
| `/Users/joshuapare/Repos/omniviewdev/omniview-ui/packages/base-ui/` | ||
|
|
||
| Create a worktree for this work before starting: | ||
|
|
||
| ```bash | ||
| cd /Users/joshuapare/Repos/omniviewdev/omniview-ui | ||
| git worktree add worktrees/omniview-ui/date-time-pickers -b feat/date-time-pickers | ||
| cd worktrees/omniview-ui/date-time-pickers | ||
| pnpm install | ||
| pnpm --filter @omniviewdev/base-ui test | ||
| pnpm --filter @omniviewdev/base-ui build | ||
| ``` |
There was a problem hiding this comment.
Replace author-local absolute paths with repo-relative instructions.
The /Users/joshuapare/... path is machine-specific and will break for other contributors or automation. Use repo-relative paths plus a $REPO_ROOT placeholder.
Proposed documentation adjustment
-**Tech Stack:** TypeScript 5.9, React 19, Vite 5, Vitest + jsdom, `@testing-library/react`, `@testing-library/user-event`, Storybook 10, `@base-ui/react`, CSS Modules. Source location: `/Users/joshuapare/Repos/omniviewdev/omniview-ui/packages/base-ui/`.
+**Tech Stack:** TypeScript 5.9, React 19, Vite 5, Vitest + jsdom, `@testing-library/react`, `@testing-library/user-event`, Storybook 10, `@base-ui/react`, CSS Modules. Source location: `packages/base-ui/`.
-All file paths in this plan are relative to the base-ui package:
-`/Users/joshuapare/Repos/omniviewdev/omniview-ui/packages/base-ui/`
+All file paths in this plan are relative to the repository root unless otherwise noted.
Create a worktree for this work before starting:
```bash
-cd /Users/joshuapare/Repos/omniviewdev/omniview-ui
+cd "$REPO_ROOT"
git worktree add worktrees/omniview-ui/date-time-pickers -b feat/date-time-pickers
cd worktrees/omniview-ui/date-time-pickers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-12-date-time-pickers.md` around lines 9 - 33,
Replace the hard-coded author-local absolute path
"/Users/joshuapare/Repos/omniviewdev/omniview-ui" in
docs/superpowers/plans/2026-04-12-date-time-pickers.md with a repo-relative
placeholder (e.g. use the variable "$REPO_ROOT") and update the Working
Directory & Worktree command sequence so the first cd uses cd "$REPO_ROOT"
before running git worktree add and the subsequent steps; ensure the example
shows the repo-relative flow around the existing git worktree add, pnpm install,
pnpm --filter `@omniviewdev/base-ui` test, and pnpm --filter `@omniviewdev/base-ui`
build commands.
Comment on lines
+104
to
+130
| For `:root[data-ov-theme='dark']`: | ||
| ```css | ||
| --ov-color-datepicker-header-bg: var(--ov-color-bg-surface-raised); | ||
| --ov-color-datepicker-cell-bg: transparent; | ||
| --ov-color-datepicker-cell-bg-today: var(--ov-color-state-selected); | ||
| --ov-color-datepicker-cell-bg-selected: var(--ov-color-brand-500); | ||
| --ov-color-datepicker-cell-bg-hover: var(--ov-color-state-hover); | ||
| --ov-color-datepicker-cell-fg: var(--ov-color-fg-default); | ||
| --ov-color-datepicker-cell-fg-today: var(--ov-color-fg-default); | ||
| --ov-color-datepicker-cell-fg-selected: var(--ov-color-fg-inverse); | ||
| --ov-color-datepicker-cell-fg-disabled: var(--ov-color-fg-disabled); | ||
| --ov-color-datepicker-cell-fg-other-month: var(--ov-color-fg-muted); | ||
| ``` | ||
|
|
||
| For the two high-contrast themes, pick values that preserve WCAG AAA contrast (e.g., `--ov-color-datepicker-cell-bg-selected: var(--ov-color-fg-default)` with inverse fg, or use the theme's pure white/black variables). | ||
|
|
||
| For `:root[data-ov-density='comfortable']`: | ||
| ```css | ||
| --ov-size-datepicker-cell: 32px; | ||
| --ov-size-datepicker-gap: 2px; | ||
| ``` | ||
|
|
||
| For `:root[data-ov-density='compact']`: | ||
| ```css | ||
| --ov-size-datepicker-cell: 26px; | ||
| --ov-size-datepicker-gap: 1px; | ||
| ``` |
There was a problem hiding this comment.
Add blank lines around fenced code blocks.
markdownlint reports MD031 for these CSS fences. Add a blank line before each opening fence so docs lint stays clean.
Proposed formatting fix
For `:root[data-ov-theme='dark']`:
+
```css
--ov-color-datepicker-header-bg: var(--ov-color-bg-surface-raised);
...For :root[data-ov-density='comfortable']:
+
--ov-size-datepicker-cell: 32px;
--ov-size-datepicker-gap: 2px;For :root[data-ov-density='compact']:
+
--ov-size-datepicker-cell: 26px;
--ov-size-datepicker-gap: 1px;</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
For `:root[data-ov-theme='dark']`:
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 105-105: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 121-121: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 127-127: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-12-date-time-pickers.md` around lines 104 -
130, The markdown has fenced CSS blocks missing a preceding blank line (causing
MD031); add a blank line before each opening ```css fence in the sections for
the theme block introduced by "For `:root[data-ov-theme='dark']`" and the two
density blocks introduced by "For `:root[data-ov-density='comfortable']`" and
"For `:root[data-ov-density='compact']`" so each fenced block is separated from
the preceding paragraph; ensure there is a blank line before each opening fence
(and keep the existing closing fences intact).
Comment on lines
+1718
to
+1735
| 1. Before the default export, register the sample: | ||
| ```typescript | ||
| import { themeRegistry } from '../src/theme/registry'; | ||
| import { SOLARIZED_DARK } from './sampleCustomThemes'; | ||
|
|
||
| if (!themeRegistry.has(SOLARIZED_DARK.id)) themeRegistry.register(SOLARIZED_DARK); | ||
| ``` | ||
| 2. The `globalTypes.theme.toolbar.items` array (if present) is replaced with an array built from `themeRegistry.list()` so custom themes appear automatically. If the existing preview uses a static list, replace it like: | ||
| ```typescript | ||
| theme: { | ||
| description: 'Theme', | ||
| defaultValue: 'dark', | ||
| toolbar: { | ||
| title: 'Theme', | ||
| items: themeRegistry.list().map((t) => ({ value: t.id, title: t.name })), | ||
| }, | ||
| }, | ||
| ``` |
There was a problem hiding this comment.
Add blank lines around the nested fenced code blocks.
The fences inside this numbered list violate MD031 and will keep markdownlint reporting warnings.
📝 Proposed formatting fix
1. Before the default export, register the sample:
+
```typescript
import { themeRegistry } from '../src/theme/registry';
import { SOLARIZED_DARK } from './sampleCustomThemes';
if (!themeRegistry.has(SOLARIZED_DARK.id)) themeRegistry.register(SOLARIZED_DARK);
```
+
2. The `globalTypes.theme.toolbar.items` array (if present) is replaced with an array built from `themeRegistry.list()` so custom themes appear automatically. If the existing preview uses a static list, replace it like:
+
```typescript
theme: {
description: 'Theme',
defaultValue: 'dark',
toolbar: {
title: 'Theme',
items: themeRegistry.list().map((t) => ({ value: t.id, title: t.name })),
},
},
```
+
3. The decorator (if already present) that sets `data-ov-theme` directly must be removed — applying via `themeRegistry.apply(context.globals.theme)` is what drives DOM state now.📝 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.
Suggested change
| 1. Before the default export, register the sample: | |
| ```typescript | |
| import { themeRegistry } from '../src/theme/registry'; | |
| import { SOLARIZED_DARK } from './sampleCustomThemes'; | |
| if (!themeRegistry.has(SOLARIZED_DARK.id)) themeRegistry.register(SOLARIZED_DARK); | |
| ``` | |
| 2. The `globalTypes.theme.toolbar.items` array (if present) is replaced with an array built from `themeRegistry.list()` so custom themes appear automatically. If the existing preview uses a static list, replace it like: | |
| ```typescript | |
| theme: { | |
| description: 'Theme', | |
| defaultValue: 'dark', | |
| toolbar: { | |
| title: 'Theme', | |
| items: themeRegistry.list().map((t) => ({ value: t.id, title: t.name })), | |
| }, | |
| }, | |
| ``` | |
| 1. Before the default export, register the sample: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1719-1719: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1724-1724: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1726-1726: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1735-1735: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-12-theme-extensibility-system.md` around lines
1718 - 1735, The markdown has fenced TypeScript blocks nested inside a numbered
list without blank lines, which triggers MD031; add a blank line before and
after each fenced code block shown (the block importing themeRegistry and
SOLARIZED_DARK and the block showing the theme global), ensuring the list items
separate from the triple-backtick fences; verify the sections referencing
themeRegistry, SOLARIZED_DARK, globalTypes.theme.toolbar.items,
themeRegistry.list(), and themeRegistry.apply(context.globals.theme) keep their
surrounding blank lines so the fences are treated as code blocks and
markdownlint stops flagging MD031.
Merging this PR will improve performance by 13.66%
Performance Changes
Comparing 
Footnotes
|
- Calendar: normalize times in range comparisons (start-of-day); clamp focusedDate into new month on prev/next - DatePicker: use shared isDateInRange; add aria-disabled for readOnly on icon button - DateRangePicker: use day-granular isBefore/isAfter; drop current.end cap on start field max - DateTimePicker: keep popover open on date select; memoize fallback Date - TimePicker: support defaultValue with internal state; guard hover/focus CSS on data-disabled - TimeColumns: dedupe Enter/Space handler at ul level (sets suppressScrollRef) - DateField: advance focus after paste via flushSync+focusSection - sections: stamp explicit hourCycle on hour Section; simplify dead format-option branches; meridiem else branch preserves tokenIdx - dateUtils: clamp addYears to target month length (Feb 29 → Feb 28) - Stories: extract useState into PascalCase wrappers across all 5 pickers - Barrel: re-export BuildSectionsOptions and ValidateSectionsResult - Showcase: double-quote pnpm filter for cross-platform build:deps - Tests: tighter assertions on shellError, minuteStep, paste year, weekday labels - Docs: add DateField/DateRangePicker to COMPONENT_STATUS
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/COMPONENT_STATUS.md (1)
23-77:⚠️ Potential issue | 🟡 MinorStatus inconsistency with
packages/base-ui/docs/COMPONENT_STATUS.md.This top-level list puts the six new date/time components into the "Approved component set" (implicitly Stable), while
packages/base-ui/docs/COMPONENT_STATUS.mdexplicitly marks themBeta[^1]pending UX verification. Consider adding a brief "(Beta)" suffix here (or a footnote) so downstream consumers reading only this document don't interpret the components as fully stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/COMPONENT_STATUS.md` around lines 23 - 77, The top-level COMPONENT_STATUS.md lists Calendar, DateField, DatePicker, DateRangePicker, DateTimePicker, and TimePicker as approved/stable while packages/base-ui marks them Beta; update this file so those six component entries (Calendar, DateField, DatePicker, DateRangePicker, DateTimePicker, TimePicker) include a "(Beta)" suffix or add a brief footnote reference next to them to match packages/base-ui/docs/COMPONENT_STATUS.md and make the status consistent for downstream readers.
♻️ Duplicate comments (1)
packages/base-ui/src/components/time-picker/TimePicker.module.css (1)
121-123:⚠️ Potential issue | 🟡 MinorSuppress hover affordance for disabled icon buttons.
The root selector was fixed, but the disabled clock button can still receive the hover color and look interactive.
Suggested fix
-.iconButton:hover { +.iconButton:not(:disabled):hover { color: var(--ov-color-fg-default); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/time-picker/TimePicker.module.css` around lines 121 - 123, The hover style on .iconButton currently applies even when the control is disabled; update the CSS so the hover color only applies to enabled buttons by scoping the rule to enabled states (e.g., .iconButton:not(:disabled):hover) and also account for aria-disabled (e.g., .iconButton:not([aria-disabled="true"]):hover) so the disabled clock icon does not gain hover affordance; modify the selector(s) for the existing .iconButton:hover rule accordingly and remove or override any conflicting rules that still allow hover styling on disabled elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/date-field/DateField.tsx`:
- Around line 19-20: The DateField component exposes a placeholder prop that is
never used; either wire it into the rendering path or remove it from the public
props. Locate the DateField props/interface (e.g., DateField or DateFieldProps)
and either (A) implement usage by passing placeholder down to the underlying
inputs/section-renderer where per-section placeholders are created (ensure the
top-level placeholder overrides or composes with per-section placeholders), or
(B) remove the placeholder declaration and any related JSDoc/exports so it is no
longer part of the public API; update any other occurrences mentioned (lines
40-54 and 105-108 areas) to match the choice.
In `@packages/base-ui/src/components/date-field/sections.ts`:
- Around line 408-414: The Date construction uses new Date(y, m, d, h, minute,
second) which remaps years 0–99 to 1900–1999; update the creation to build the
Date with the same components but then call date.setFullYear(y) to restore the
intended year (use the existing variables y, m, d, h, minute, second and the
date variable), and afterwards verify the resulting date's components
(getFullYear/getMonth/getDate/getHours/etc.) match y/m/d/h to ensure no rolling
occurred; adjust any downstream logic that relies on the original date if
validation fails.
In `@packages/base-ui/src/components/date-field/useDateField.ts`:
- Around line 123-139: The rebuild key only compares section types so changes to
separators/widths/placeholders/hour metadata don't trigger rebuilding; update
the key logic (currentTemplateKey and lastTemplateKey initialization) to
serialize the full template metadata instead of just types (e.g.,
JSON.stringify(template) or stringify each section's type, text, width,
placeholder, hourCycle, etc.), so useEffect detects template-detail changes and
calls setSections(buildSections(...)) accordingly, referencing
currentTemplateKey, lastTemplateKey, template, buildSections, setSections, and
value.
- Around line 34-35: The public options min and max on useDateField are never
used, so out-of-range selections still yield isValid=true and still trigger
onChange; update the validation path (e.g., the validate/normalize function or
wherever isValid is computed inside useDateField) to check the selected Date
against the min and max fields (min?: Date and max?: Date) and set isValid=false
when selected < min or > max, and ensure onChange is suppressed or given an
invalid flag when isValid is false, or alternatively remove min/max from the
public options if you prefer not to support bounds—adjust the logic in the
functions that compute isValid and call onChange (referencing useDateField,
isValid, onChange, min, max) accordingly.
In `@packages/base-ui/src/components/date-picker/Calendar.module.css`:
- Around line 62-70: The disabled selector .cell[aria-disabled='true'] doesn't
reset background so a cell that is both selected and disabled can still show the
selected background; update the .cell[aria-disabled='true'] rule to explicitly
set background: var(--ov-color-datepicker-cell-bg) (in addition to the existing
color and cursor) so disabled cells always use the default background and never
appear selected, ensuring consistency with .cell[aria-selected='true'] and
resolving the specificity/cascade conflict.
In `@packages/base-ui/src/components/date-picker/Calendar.tsx`:
- Around line 155-161: moveFocus currently sets setFocusedDate and schedules a
microtask that may run before the month re-render, causing querySelector on
gridRef to return null and focus to be lost; change to update viewMonth and
focusedDate together when crossing month boundaries and remove the microtask
focus, then add a useLayoutEffect/useEffect that watches focusedDate (and
viewMonth if needed) and performs
gridRef.current?.querySelector(`[data-date='${toIsoDay(focusedDate)}']`)?.focus();
ensure the effect runs after the DOM commit so the target cell is rendered and
reference the existing functions/refs (moveFocus, setFocusedDate, viewMonth,
setViewMonth, gridRef, toIsoDay, focusedDate, rovingRef) to locate where to
update state and where to place the focus logic.
In `@packages/base-ui/src/components/date-picker/DatePicker.module.css`:
- Around line 152-154: The CSS selector ".footer > :first-child { margin-right:
auto; }" uses a physical property that breaks in RTL; replace the physical
margin with the logical equivalent (margin-inline-end: auto) so the first child
is pushed to the start in LTR and to the start in RTL. Update the rule in
DatePicker.module.css for the selector ".footer > :first-child" to use the
logical property and remove or stop using margin-right to ensure RTL-safe
layout.
In `@packages/base-ui/src/components/date-picker/dateUtils.test.ts`:
- Around line 88-95: The final assertion in the test for getMonthMatrix is too
loose; replace the range check on matrix[5]![6] with a strict equality check
against the deterministic expected date for April 2026 when weekStartsOn=0 (new
Date(2026, 4, 9)); update the assertion in the test case that references
matrix[5]![6] to use toEqual(new Date(2026, 4, 9)) so the test verifies the
exact last-cell value from getMonthMatrix.
In `@packages/base-ui/src/components/date-picker/dateUtils.ts`:
- Around line 54-58: clampDate currently returns the caller-supplied min/max
Date objects by reference when d is out of bounds; change the return values to
copies to avoid mutating the original bounds (use new Date(min) and new
Date(max) instead of returning min/max directly) so clampDate always returns a
fresh Date instance consistent with the other helpers.
In `@packages/base-ui/src/components/date-picker/formatters.ts`:
- Around line 20-37: The fallback currently treats every non-en-US locale as
Monday; update getWeekStartsOnForLocale to consult a small hard-coded
locale-to-weekStart map for known exceptions (e.g., ar-SA => 6 (Saturday), he-IL
=> 0 (Sunday), pt-BR => 0 (Sunday)) before returning the generic Monday
fallback. Normalize the incoming locale tag to lowercase and check both exact
tags and prefix matches (startsWith) against keys in the map, then return the
mapped WeekStart when found; otherwise preserve the existing en-US special-case
and final default of 1.
In `@packages/base-ui/src/components/date-range-picker/DateRangePicker.tsx`:
- Around line 185-196: The calendar toggle button in DateRangePicker.tsx
currently sets disabled={disabled} but omits an ARIA indicator for readOnly;
update the button element (the one rendering LuCalendarRange) to add
aria-disabled={readOnly || undefined} so assistive tech will announce its
non-interactive state when readOnly is true while keeping disabled controlled by
the disabled prop.
- Around line 173-184: The end DateField's min prop currently reads
min={current.start ?? min}, which prevents typed end-dates earlier than the
current start from reaching handleEndChange and makes the "swap on isBefore"
branch in handleEndChange unreachable; change the end field to use the
unconditional min (same approach as the start field) — e.g., set min to min (not
current.start) or remove the current.start cap so typed input is allowed to
trigger handleEndChange's swap logic (also update or remove any comment
referring to the cap to match the start-field decision).
In `@packages/base-ui/src/components/date-time-picker/DateTimePicker.module.css`:
- Around line 85-87: The .iconButton hover rule is applying even when the
trigger is disabled; update the selector(s) so hover styling only applies to
enabled buttons (e.g., change .iconButton:hover to
.iconButton:not(:disabled):hover and also add variants to ignore .disabled class
and aria-disabled like .iconButton:not(.disabled):hover and
.iconButton[aria-disabled="true"] should not get hover styles) so disabled
triggers do not receive the active hover color; modify the
DateTimePicker.module.css rules referencing .iconButton accordingly.
In `@packages/base-ui/src/components/date-time-picker/DateTimePicker.tsx`:
- Around line 53-57: The local isDateInRange function duplicates shared
dateUtils but uses raw Date comparisons (time-aware) rather than date-only
semantics; either add a clear comment above isDateInRange explaining that it
intentionally compares full timestamps for datetime semantics, or better:
implement a new isDateTimeInRange in the dateUtils module (preserving
startOfDay-based isDateInRange there), export it, and replace the local
isDateInRange with an import of dateUtils.isDateTimeInRange so both semantics
live in one place and callers are explicit about datetime vs date-only behavior.
- Around line 160-171: The calendar trigger button in DateTimePicker does not
expose readOnly to assistive tech; update the <button> element in DateTimePicker
(the calendar icon trigger) to include aria-disabled={readOnly || undefined}
(matching DatePicker behavior) so AT announces non-interactivity when readOnly
is true, while keeping the existing disabled={disabled} prop and the onClick
guard that checks !disabled && !readOnly.
In `@packages/base-ui/src/components/time-picker/TimeColumns.tsx`:
- Around line 128-148: The TimeColumns component currently merges readOnly into
isInteractionDisabled (disabled || readOnly) which is passed down as the child
disabled prop and makes options unfocusable; change the logic so only the
explicit disabled prop controls the child disabled attribute (i.e., compute
isInteractionDisabled = disabled) and do not forward readOnly as disabled to
children; instead keep passing readOnly into TimeColumns and rely on the
existing handleXxxSelect / onSelect early-returns to prevent commits while
preserving focusability of <li> options. Ensure any place using
isInteractionDisabled (or passing disabled to option elements) is updated to use
only disabled, and keep readOnly available for conditional selection logic.
In `@packages/base-ui/src/components/time-picker/TimePicker.test.tsx`:
- Around line 194-205: The test "Clear button resets the time to midnight" can
falsely pass if Clear sets "today at midnight" instead of preserving the
original calendar date; update the test for the TimePicker to assert that the
onChange Date returned by clicking the 'Clear' button preserves the original
year, month and day (compare called.getFullYear(), called.getMonth(),
called.getDate() against the original Date(2026, 3, 12)), in addition to
checking hours/minutes/seconds are zero so the clear action truly resets time
but keeps the calendar date.
In `@packages/base-ui/src/components/time-picker/TimePicker.tsx`:
- Around line 127-137: The icon button remains focusable and announced when
readOnly is true but its handler ignores activation; make its interactivity
match non-opening behavior by treating readOnly like disabled: change the button
props so disabled={disabled || readOnly}, remove or set tabIndex to undefined/-1
when readOnly, add aria-disabled={readOnly} for a11y, and avoid wiring
handleIconButtonClick when readOnly (e.g., onClick={readOnly ? undefined :
handleIconButtonClick}); update references around iconButtonRef,
handleIconButtonClick, open, popoverId and styles.iconButton accordingly.
---
Outside diff comments:
In `@docs/COMPONENT_STATUS.md`:
- Around line 23-77: The top-level COMPONENT_STATUS.md lists Calendar,
DateField, DatePicker, DateRangePicker, DateTimePicker, and TimePicker as
approved/stable while packages/base-ui marks them Beta; update this file so
those six component entries (Calendar, DateField, DatePicker, DateRangePicker,
DateTimePicker, TimePicker) include a "(Beta)" suffix or add a brief footnote
reference next to them to match packages/base-ui/docs/COMPONENT_STATUS.md and
make the status consistent for downstream readers.
---
Duplicate comments:
In `@packages/base-ui/src/components/time-picker/TimePicker.module.css`:
- Around line 121-123: The hover style on .iconButton currently applies even
when the control is disabled; update the CSS so the hover color only applies to
enabled buttons by scoping the rule to enabled states (e.g.,
.iconButton:not(:disabled):hover) and also account for aria-disabled (e.g.,
.iconButton:not([aria-disabled="true"]):hover) so the disabled clock icon does
not gain hover affordance; modify the selector(s) for the existing
.iconButton:hover rule accordingly and remove or override any conflicting rules
that still allow hover styling on disabled elements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4e62767-b72c-4931-ab61-5b3a2da2f6e0
📒 Files selected for processing (43)
apps/showcase/package.jsondocs/COMPONENT_STATUS.mdpackages/base-ui/docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/date-field/DateField.module.csspackages/base-ui/src/components/date-field/DateField.stories.tsxpackages/base-ui/src/components/date-field/DateField.test.tsxpackages/base-ui/src/components/date-field/DateField.tsxpackages/base-ui/src/components/date-field/index.tspackages/base-ui/src/components/date-field/sections.test.tspackages/base-ui/src/components/date-field/sections.tspackages/base-ui/src/components/date-field/useDateField.tspackages/base-ui/src/components/date-picker/Calendar.module.csspackages/base-ui/src/components/date-picker/Calendar.test.tsxpackages/base-ui/src/components/date-picker/Calendar.tsxpackages/base-ui/src/components/date-picker/DatePicker.module.csspackages/base-ui/src/components/date-picker/DatePicker.stories.tsxpackages/base-ui/src/components/date-picker/DatePicker.test.tsxpackages/base-ui/src/components/date-picker/DatePicker.tsxpackages/base-ui/src/components/date-picker/dateUtils.test.tspackages/base-ui/src/components/date-picker/dateUtils.tspackages/base-ui/src/components/date-picker/formatters.test.tspackages/base-ui/src/components/date-picker/formatters.tspackages/base-ui/src/components/date-picker/index.tspackages/base-ui/src/components/date-range-picker/DateRangePicker.module.csspackages/base-ui/src/components/date-range-picker/DateRangePicker.stories.tsxpackages/base-ui/src/components/date-range-picker/DateRangePicker.test.tsxpackages/base-ui/src/components/date-range-picker/DateRangePicker.tsxpackages/base-ui/src/components/date-range-picker/index.tspackages/base-ui/src/components/date-time-picker/DateTimePicker.module.csspackages/base-ui/src/components/date-time-picker/DateTimePicker.stories.tsxpackages/base-ui/src/components/date-time-picker/DateTimePicker.test.tsxpackages/base-ui/src/components/date-time-picker/DateTimePicker.tsxpackages/base-ui/src/components/date-time-picker/index.tspackages/base-ui/src/components/editor-tabs/hooks/useTabDetach.test.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/sortable-table/useSortableTable.test.tspackages/base-ui/src/components/time-picker/TimeColumns.tsxpackages/base-ui/src/components/time-picker/TimePicker.module.csspackages/base-ui/src/components/time-picker/TimePicker.stories.tsxpackages/base-ui/src/components/time-picker/TimePicker.test.tsxpackages/base-ui/src/components/time-picker/TimePicker.tsxpackages/base-ui/src/components/time-picker/index.tspackages/base-ui/src/theme/styles.css
Comment on lines
+123
to
+139
| // Track the last "external" value we synced from, to avoid clobbering local edits. | ||
| const lastSyncedValue = useRef<Date | null>(value); | ||
| const lastTemplateKey = useRef<string>(JSON.stringify(template.map((s) => s.type))); | ||
|
|
||
| const currentTemplateKey = useMemo( | ||
| () => JSON.stringify(template.map((s) => s.type)), | ||
| [template], | ||
| ); | ||
|
|
||
| // When the template changes (mode/locale/hourCycle/showSeconds), rebuild from value. | ||
| useEffect(() => { | ||
| if (lastTemplateKey.current !== currentTemplateKey) { | ||
| setSections(buildSections({ mode, locale, hourCycle, showSeconds, value })); | ||
| lastTemplateKey.current = currentTemplateKey; | ||
| lastSyncedValue.current = value; | ||
| } | ||
| }, [currentTemplateKey, mode, locale, hourCycle, showSeconds, value]); |
There was a problem hiding this comment.
Include full section template metadata in the rebuild key.
currentTemplateKey only tracks section types. A locale change that keeps the same type order but changes separators, widths, placeholders, or hour metadata will not rebuild sections when value is unchanged.
🛠️ Suggested key expansion
- const lastTemplateKey = useRef<string>(JSON.stringify(template.map((s) => s.type)));
+ const templateKey = useCallback(
+ (items: Section[]) =>
+ JSON.stringify(
+ items.map((s) => ({
+ type: s.type,
+ literal: s.type === 'literal' ? s.value : undefined,
+ placeholder: s.placeholder,
+ maxLength: s.maxLength,
+ min: s.min,
+ max: s.max,
+ hourCycle: s.hourCycle,
+ })),
+ ),
+ [],
+ );
+
+ const lastTemplateKey = useRef<string>(templateKey(template));
const currentTemplateKey = useMemo(
- () => JSON.stringify(template.map((s) => s.type)),
- [template],
+ () => templateKey(template),
+ [template, templateKey],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/date-field/useDateField.ts` around lines 123
- 139, The rebuild key only compares section types so changes to
separators/widths/placeholders/hour metadata don't trigger rebuilding; update
the key logic (currentTemplateKey and lastTemplateKey initialization) to
serialize the full template metadata instead of just types (e.g.,
JSON.stringify(template) or stringify each section's type, text, width,
placeholder, hourCycle, etc.), so useEffect detects template-detail changes and
calls setSections(buildSections(...)) accordingly, referencing
currentTemplateKey, lastTemplateKey, template, buildSections, setSections, and
value.
Comment on lines
+443
to
+450
| return { | ||
| ...base, | ||
| role: 'spinbutton', | ||
| 'aria-label': ariaLabelForSection(section.type), | ||
| 'aria-readonly': !!readOnly, | ||
| contentEditable: disabled || readOnly ? 'false' : 'true', | ||
| suppressContentEditableWarning: true, | ||
| tabIndex: isFocused ? 0 : -1, |
There was a problem hiding this comment.
Keep one editable section tabbable when the field is idle.
When focusedIndex is null, every section receives tabIndex: -1 and the root has no tabIndex, so keyboard users cannot tab into the field.
♿ Proposed fix
const isFocused = focusedIndex === index;
const isLiteral = section.type === 'literal';
const isPlaceholder = !isLiteral && section.value === '';
+ const firstEditableIndex = getEditableIndices(sections)[0] ?? null;
+ const tabbableIndex = focusedIndex ?? firstEditableIndex;- tabIndex: isFocused ? 0 : -1,
+ tabIndex: !disabled && index === tabbableIndex ? 0 : -1,- sections: setFullYear to avoid year 0-99 remapping - Calendar: reset background on disabled cells; useLayoutEffect for post-focus commit - DateRangePicker: drop current.start cap on end field min; aria-disabled on icon - DateTimePicker: aria-disabled for readOnly; hover only when enabled - TimePicker: full readOnly disabling of icon button; hover only when enabled - TimeColumns: readOnly doesn't disable option focus - DateField: remove unused placeholder prop - useDateField: enforce min/max in validation - dateUtils: clampDate returns copies of min/max - formatters: locale-specific weekStartsOn map - CSS: margin-inline-end for RTL-safe footer layout - Tests: tighter getMonthMatrix assertion; Clear preserves calendar date - Docs: mark pickers as Beta in top-level COMPONENT_STATUS
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
1 participant
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.
Summary
Adds a full suite of date/time picker components to
@omniviewdev/base-ui, built on@base-ui/reactprimitives + CSS Modules with a shared sectioned guided-input primitive (DateField) that mirrors MUI's field UX.Components
Calendar— month grid with single & range modes, full WAI-ARIA keyboard nav (arrows, Home/End, PageUp/Down, Shift+PageUp/Down, Enter, Escape)DateField— sectioned input primitive: tabbable sections (MM/DD/YYYY, HH:mm AM/PM, etc.), per-section validation (day bounded by month+year, leap-year aware), arrow-key increment/decrement, auto-advance on max-length, paste-to-fill, Intl-locale-aware format tokenizationDatePicker— DateField trigger + calendar popoverTimePicker— DateField trigger + scrollable column popover (hour/minute/seconds/AM-PM)DateTimePicker— combined date+time DateField trigger + side-by-side calendar & columns popupDateRangePicker— dual DateField triggers + range-selecting Calendar popoverTimeColumns— extracted reusable column-selector primitive shared between TimePicker & DateTimePickerAll pickers include a Clear / Done footer in the popover.
Theming
--ov-color-datepicker-*semantic tokens defined per-theme across all 7 built-ins--ov-size-datepicker-cell,--ov-size-datepicker-gap)Architecture notes
Date+Intl.DateTimeFormat/Intl.Localebaremode on DateField that strips its own border/bg when embedded in a picker's shell, avoiding doubled focus ringsCalendarvia discriminated union (mode: 'single' | 'range') with in-range highlighting + hover previewPopoverwrapper (not raw @base-ui/react) so z-index, animation states, and portal styling are consistent with the rest of the libraryTest plan
Build ergonomics
Coordination with theme-extensibility
This PR is independent of `feat/theme-extensibility`. When both merge, run `pnpm --filter @omniviewdev/base-ui generate:token-keys` and commit the refreshed `tokenKeys.ts` so custom themes can override the new `color.datepicker.*` tokens via the typed key union.
Summary by CodeRabbit
New Features
Documentation
Chores