Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #1187 by ensuring DateRangePicker does not default-select today when value is undefined, and it adds UX tweaks for empty/validation states plus demo controls to quickly clear/set a range.
Changes:
- Reset internal picker state when
valuebecomesundefinedso placeholders render correctly instead of selecting today. - Adjust date/time input behaviors for empty/partial selections and validation neutrality.
- Update the site demo to allow toggling
valuebetweenundefinedand a preset range.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/blend/lib/components/DateRangePicker/utils.ts | Removes hardcoded debug date parsing; changes date-input parsing/clamping and end-date behavior. |
| packages/blend/lib/components/DateRangePicker/TimeSelector.tsx | Changes time selector open/disabled behavior in empty/partial-selection states. |
| packages/blend/lib/components/DateRangePicker/DateRangePicker.tsx | Resets internal state when value is cleared; tweaks empty end-date validation handling. |
| apps/site/src/demos/DateRangePickerDemo.tsx | Adds buttons to clear/set the picker value and renders the “undefined” state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const dateForCheck = parseDate(formattedValue, dateFormat, 12, 0) | ||
| const date = | ||
| dateForCheck && isValidDate(dateForCheck) | ||
| ? dateForCheck | ||
| : new Date() | ||
| const endDateTimeCheck = |
There was a problem hiding this comment.
handleDateInputChange now sets date to new Date() when parseDate(...) fails. That can incorrectly treat an unparsable/invalid input as “today” and clamp hours/minutes for past/future restrictions. Prefer deriving endDateTimeCheck/startDateTimeCheck only when dateForCheck is non-null and valid (otherwise treat as not-today).
| ? { ...currentRange, startDate: parsedDate } | ||
| : { startDate: parsedDate } | ||
| : currentRange | ||
| ? { ...currentRange, endDate: parsedDate } | ||
| : undefined | ||
| : { startDate: parsedDate, endDate: parsedDate } |
There was a problem hiding this comment.
When changing the end-date input with no existing currentRange, this now creates a range { startDate: parsedDate, endDate: parsedDate }. This implicitly sets a start date the user didn’t choose (and can enable downstream logic based on selectedRange), leading to inconsistent UI state (start input still empty) and potentially committing an unintended range. Consider keeping updatedRange undefined in this case, or preventing end-date edits until a start date exists, or storing an “incomplete range” state separately instead of fabricating a start date.
| * @param currentRange Current selected range | ||
| * @param timeValue Current time value (HH:mm) | ||
| * @param isStartDate Whether this is start date or end date | ||
| * @param disableFutureDates Whether future dates should be disabled | ||
| * @param disablePastDates Whether past dates should be disabled | ||
| * @returns Object with formatted value, validation result, and updated range | ||
| */ | ||
| export const handleDateInputChange = ( | ||
| value: string, | ||
| dateFormat: string, | ||
| currentRange: DateRange | undefined, | ||
| timeValue?: string, | ||
| _timeValue?: string, | ||
| isStartDate: boolean = true, |
There was a problem hiding this comment.
The timeValue parameter is now unused (renamed to _timeValue) but the JSDoc still documents it as an active input. Either remove the parameter (and update call sites) or update the docstring to avoid misleading future changes.
| id={id} | ||
| type="text" | ||
| disabled={ | ||
| isStart && selectedRange | ||
| ? !selectedRange.startDate | ||
| : !selectedRange?.endDate | ||
| isStart | ||
| ? false | ||
| : !!selectedRange && !selectedRange.endDate | ||
| } |
There was a problem hiding this comment.
With the new disabled logic, both time inputs can become enabled even when no corresponding date is selected (e.g. selectedRange is undefined makes the end-time input enabled too). In that state, TimeSelector can accept/format a time, but the parent handlers bail out when selectedRange is missing, so the UI can show a time that isn't actually stored/applied (and Apply remains disabled). Consider restoring the previous gating (disable and prevent menu opening until the relevant date exists) or updating the parent to persist time independently; also ensure TimeSelector clears its internal inputValue when value becomes empty to avoid stale times after clearing.
| useEffect(() => { | ||
| if (!value) { | ||
| lastExternalValueRef.current = null | ||
| resetValues(undefined) | ||
| return |
There was a problem hiding this comment.
The new value === undefined reset behavior is a regression-prone path (and fixes Issue #1187). There are existing DateRangePicker test suites; please add a regression test that renders with value={undefined} and asserts the trigger/input shows the placeholder and that today is not marked as selected by default.
Summary
when value is undefined, the picker no longer shows today’s date as selected and correctly shows the placeholder. Also adds several UX improvements for the empty state and validation.
Issue Ticket
Closes #1187