Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix end time selection before selecting date #6858

Merged
merged 8 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/datetime2/changelog/@unreleased/pr-6858.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: Fix `DateRangePicker` end time selection before selecting end date
links:
- https://github.com/palantir/blueprint/pull/6858
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import classNames from "classnames";
import { format } from "date-fns";
import { addDays, format } from "date-fns";
import * as React from "react";
import type { DateFormatter, DayModifiers, DayMouseEventHandler, ModifiersClassNames } from "react-day-picker";

Expand Down Expand Up @@ -265,17 +265,38 @@ export class DateRangePicker3 extends DateFnsLocalizedComponent<DateRangePicker3

const { value, time } = this.state;
const newValue = DateUtils.getDateTime(
value[dateIndex] != null ? DateUtils.clone(value[dateIndex]!) : new Date(),
value[dateIndex] != null ? DateUtils.clone(value[dateIndex]!) : this.getDefaultDate(dateIndex),
newTime,
);
const newDateRange: DateRange = [value[0], value[1]];
newDateRange[dateIndex] = newValue;
const newTimeRange: DateRange = [time[0], time[1]];
newTimeRange[dateIndex] = newTime;

this.props.onChange?.(newDateRange);
this.setState({ value: newDateRange, time: newTimeRange });
};

// When a user sets the time value before choosing a date, we need to pick a date for them
// The default depends on the value of the other date since there's an invariant
// that the left/0 date is always less than the right/1 date
private getDefaultDate = (dateIndex: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a suggestion, I think we can add an inline source code comment to explain why we need to be careful about selecting the next default date.

Maybe something around:

  1. When someone sets the time value before choosing a date, we need to "infer" or pick a date for them to initialize.
  2. The next default date for the 0 or 1 index depends on the value of the other index since there's an invariant that the left/0 date is always less than the right/1 date.

evansjohnson marked this conversation as resolved.
Show resolved Hide resolved
const { value } = this.state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I'm not too worried about this since this is already happening in the existing code, but I wonder if we need to grab existing state in the setState callback rather than reading directly off of this.state.

Otherwise we could end up with weird bugs if this.state is referring to stale values.

In general, any time "next state" is computed from some kind of prior state, I usually read the prior state in a setState callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof yea, I remember dealing with stuff like this back when seeing class components more often

in this case I'm thinking let's leave as is since as you call out we access this.state just before calling this helper method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree! Let's leave as-is. I almost didn't make this comment because it was an existing issue.

const otherIndex = dateIndex === 0 ? 1 : 0;
const otherDate = value[otherIndex];
if (otherDate == null) {
return new Date();
}

const { allowSingleDayRange } = this.props;
if (!allowSingleDayRange) {
const dateDiff = dateIndex === 0 ? -1 : 1;
return addDays(otherDate, dateDiff);
}

return otherDate;
};

private handleTimeChangeLeftCalendar = (time: Date) => {
this.handleTimeChange(time, 0);
};
Expand Down
28 changes: 26 additions & 2 deletions packages/datetime2/test/components/dateRangePicker3Tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { assert } from "chai";
import { format, parse } from "date-fns";
import { addDays, format, parse } from "date-fns";
import enUSLocale from "date-fns/locale/en-US";
import { mount, type ReactWrapper } from "enzyme";
import * as React from "react";
Expand Down Expand Up @@ -1293,11 +1293,35 @@ describe("<DateRangePicker3>", () => {
assert.equal(parseInt(minuteInputText, 10), newLeftMinute);
});

it("changing time without date uses today", () => {
it("changing time without date uses today, when other date not selected", () => {
render({ timePrecision: "minute" }).setTimeInput("minute", "left", 45);
assert.isTrue(DateUtils.isSameDay(onChangeSpy.firstCall.args[0][0] as Date, new Date()));
});

it("changing time without date uses other date if selected and `allowSingleDayRange` is true", () => {
const dateRange = render({ timePrecision: "minute", allowSingleDayRange: true });

dateRange.left.clickDay(10);
dateRange.setTimeInput("minute", "right", 45);
const [start, end] = dateRange.wrapper.state("value");

assert.isNotNull(start);
assert.isNotNull(end);
assert.isTrue(DateUtils.isSameDay(start!, end!));
});

it("changing time without date uses 1 day offset from other date if selected and `allowSingleDayRange` is false", () => {
const dateRange = render({ timePrecision: "minute", allowSingleDayRange: false });

dateRange.left.clickDay(10);
dateRange.setTimeInput("minute", "right", 45);
const [start, end] = dateRange.wrapper.state("value");

assert.isNotNull(start);
assert.isNotNull(end);
assert.isTrue(DateUtils.isSameDay(end!, addDays(start!, 1)));
});

it("clicking a shortcut with includeTime=false doesn't change time", () => {
render({ timePrecision: "minute", defaultValue: defaultRange }).clickShortcut();
assert.isTrue(DateUtils.isSameTime(onChangeSpy.firstCall.args[0][0] as Date, defaultRange[0]));
Expand Down