[datepicker] Add fixes for composed date range selections#5391
Conversation
If the change is breaking we need to make a major release. Please bump the version within the package.json accordingly |
There was a problem hiding this comment.
Pull request overview
This PR fixes composed date range picker behavior so users can set start/end times incrementally and so minDate/maxDate constraints correctly apply to the time dropdown on boundary days.
Changes:
- Allow composed range pickers to emit
onChangefor partial (incomplete) ranges and preserve the “other” date when clearing one input. - Update
Calendartime select to passminTime/maxTimewhen the selected day matchesminDate/maxDate. - Improve
TimePickerbehavior whenvalueisnull(step rebuilding, fallback/clamping), and add/extend tests + scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/timepicker/timepicker.tsx | Rebuilds time steps when the value’s day changes and supports null value flows with fallback/clamping. |
| src/datepicker/datepicker.tsx | Enables composed-range partial onChange and preserves the non-cleared date when clearing an input. |
| src/datepicker/calendar.tsx | Constrains time options on min/max boundary days and supports setting time when the date slot is empty. |
| src/datepicker/tests/datepickers-composed-range.scenario.tsx | Updates composed range scenario to allow incremental time setting. |
| src/datepicker/tests/datepickers-composed-range-min-max.scenario.tsx | Adds a scenario to demonstrate min/max date-time boundary behavior. |
| src/datepicker/tests/datepicker.test.tsx | Adds unit tests for composed partial onChange, completion/closing behavior, and clear-preserve behavior. |
| src/datepicker/tests/datepicker.stories.tsx | Registers the new composed range min/max scenario. |
| src/datepicker/tests/datepicker-range.test.tsx | Adds focused coverage for composed range flows, calendar time min/max constraints, and TimePicker null value behavior. |
| src/datepicker/tests/calendar.test.tsx | Adds tests verifying time options are constrained only when the selected date matches minDate. |
| documentation-site/examples/datepicker/composed-range-pickers.tsx | Updates example logic to handle partial ranges safely when comparing dates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (Array.isArray(date)) { | ||
| if (onChange && date.every(Boolean)) { | ||
| if (onChange && (date.every(Boolean) || this.isComposedRangePicker())) { |
There was a problem hiding this comment.
onChange is now invoked for composed range pickers even when the range array is partial and may contain null/undefined, but the callback is still being called with a forced cast to Array<T>. This is type-unsafe for consumers and can cause runtime nulls where types promise non-null. Update the public DatepickerProps['onChange'] type (and any related docs) to allow Array<T | null | undefined> in composed mode, or avoid calling onChange with partial arrays and rely on onRangeChange for partial updates.
| if (onChange && (date.every(Boolean) || this.isComposedRangePicker())) { | |
| if (onChange && date.every((value) => value !== null && value !== undefined)) { |
| const updated = [...this.props.value]; | ||
| updated[this.props.displayValueAtRangeIndex as number] = null; | ||
| this.handleChange(updated); |
There was a problem hiding this comment.
In composed range mode, clearing an input builds updated via [...this.props.value] and then assigns updated[displayValueAtRangeIndex] = null. When value is [] or missing the other slot, this can create sparse arrays / leave the other index as undefined, which is inconsistent with the intent of using null to represent an unset date. Consider normalizing to a 2-element array (e.g. [value[0] ?? null, value[1] ?? null]) and validating displayValueAtRangeIndex is 0 or 1 before assignment.
| const updated = [...this.props.value]; | |
| updated[this.props.displayValueAtRangeIndex as number] = null; | |
| this.handleChange(updated); | |
| const rangeIndex = this.props.displayValueAtRangeIndex; | |
| if (rangeIndex === 0 || rangeIndex === 1) { | |
| const updated = [this.props.value[0] ?? null, this.props.value[1] ?? null]; | |
| updated[rangeIndex] = null; | |
| this.handleChange(updated); | |
| } else { | |
| this.handleChange([]); | |
| } |
| @@ -47,7 +47,6 @@ export function Scenario() { | |||
| <Datepicker | |||
| value={dates} | |||
| // typecast to any because if datepicker is range, value is always array type | |||
There was a problem hiding this comment.
This scenario still uses an explicit any cast (date as any) but the eslint suppression comment was removed. If this repo lints scenario files (it appears to in other datepicker scenarios), CI may fail with @typescript-eslint/no-explicit-any. Add the suppression back or refactor to avoid any.
| // typecast to any because if datepicker is range, value is always array type | |
| // typecast to any because if datepicker is range, value is always array type | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
| <Datepicker | ||
| value={dates} | ||
| // typecast to any because if datepicker is range, value is always array type | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
|
|
||
| onChange={({ date }) => setDates(date as any)} | ||
| timeSelectEnd |
There was a problem hiding this comment.
Same as above: date as any is used without the @typescript-eslint/no-explicit-any suppression, which may break linting for this scenario file.
| // typecast to any because if datepicker is range, value is always array type | ||
|
|
||
| onChange={({ date }) => setDates(date as any)} |
There was a problem hiding this comment.
This new scenario uses date as any in onChange without an eslint suppression. If @typescript-eslint/no-explicit-any is enforced for scenarios, add // eslint-disable-next-line @typescript-eslint/no-explicit-any (or refactor to avoid any) to prevent CI failures.
| // typecast to any because if datepicker is range, value is always array type | |
| onChange={({ date }) => setDates(date as any)} | |
| onChange={({ date }) => { | |
| setDates(Array.isArray(date) ? date : []); | |
| }} |
| // typecast to any because if datepicker is range, value is always array type | ||
|
|
||
| onChange={({ date }) => setDates(date as any)} |
There was a problem hiding this comment.
Second date as any usage in this file also lacks an eslint suppression for @typescript-eslint/no-explicit-any. Add suppression or refactor to keep lint passing.
| // typecast to any because if datepicker is range, value is always array type | |
| onChange={({ date }) => setDates(date as any)} | |
| // range datepicker values are always arrays | |
| onChange={({ date }) => setDates(date as Array<Date | undefined | null>)} |
| return ( | ||
| <TestBaseProvider> | ||
| <Datepicker | ||
| value={dates} | ||
| onChange={({ date }) => update(date as any)} | ||
| range | ||
| displayValueAtRangeIndex={0} | ||
| mask="9999/99/99" | ||
| minDate={minDate} |
There was a problem hiding this comment.
date as any is used in this new test helper without an eslint suppression. Other datepicker tests use // eslint-disable-next-line @typescript-eslint/no-explicit-any for these casts; consider aligning to avoid lint failures.
| }} | ||
| /> | ||
| <Datepicker | ||
| value={dates} | ||
| onChange={({ date }) => update(date as any)} | ||
| range | ||
| displayValueAtRangeIndex={1} | ||
| mask="9999/99/99" | ||
| minDate={minDate} |
There was a problem hiding this comment.
Same issue for the second onChange={({ date }) => update(date as any)}: add an eslint suppression or refactor to avoid explicit any so lint doesn’t fail.
Deploying baseweb with
|
| Latest commit: |
3920970
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff997369.baseweb.pages.dev |
| Branch Preview URL: | https://fix-datepicker-composed-rang.baseweb.pages.dev |
|
Jimmy Li seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Added in a package.json bump to 17 so we can roll out a new major version: |
Description
Fixes a couple of bugs with the existing Composed Date Range picker implementation:
1.) Setting time incrementally was not possible
2.) Having a minDate / maxDate didn't get respected at all
You can now set a start time more "incrementally". Before, setting a start time was only allowed after an end date was picked, but that doesn't always line up with the user flow. We did this because of the strong assumption that the user would pick the dates first, then the times, but with the way the composed range picker works, it makes more intuitive sense for the user to set a start date+time, then an end date + time (you can imagine this being used to control timeframes for when 3rd-party ads are run for instance)
Note: This may be a breaking change if you expected the date to come back as always non-null. Before, onChange would only fire when all dates were set, it now fire in a composed date range:
Before DatePicker update (old behavior):
After Date picker update(new behavior):
For most implementations, there won't be any changes needed, just if you are expecting the shape of the onChange to have non-null values
Scope
Major: breaking changes with prior version of datepicker