Skip to content

Allow shortcuts to change time in Date Range Picker #3223

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Allow shortcuts to change time in Date Range Picker #3223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f023a10
Allow date range shortcuts to change time
Dec 10, 2018
6cbd464
Address PR comments
Dec 11, 2018
badae4f
lint
Dec 11, 2018
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
29 changes: 28 additions & 1 deletion packages/datetime/src/dateRangePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ import { TimePicker } from "./timePicker";
export interface IDateRangeShortcut {
label: string;
dateRange: DateRange;

/**
* By default, clicking a shortcut does not change the time of the date range picker, but instead
* takes the date components of the `dateRange` and combines it with the currently selected time.
* Setting `shouldChangeTime` to `true` will override this behavior and allow shortcuts to change
* the selected time as well.
* @default false
*/
shouldChangeTime?: boolean;
Copy link
Contributor

@giladgray giladgray Jan 15, 2019

Choose a reason for hiding this comment

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

thoughts on calling this includeTime? clarifies that the default behavior ignores time components and you must enable this field to respect them.

Copy link

@SaenkoM SaenkoM May 27, 2019

Choose a reason for hiding this comment

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

Though prop name is shouldChangeTime in PR, it was indeed changed to includeTime somewhere later (for those who will try it)

}

export interface IDateRangePickerProps extends IDatePickerBaseProps, IProps {
Expand Down Expand Up @@ -282,7 +291,7 @@ export class DateRangePicker extends AbstractPureComponent<IDateRangePickerProps
<Shortcuts
key="shortcuts"
{...{ allowSingleDayRange, maxDate, minDate, shortcuts }}
onShortcutClick={this.handleNextState}
onShortcutClick={this.handleShortcutClick}
/>,
<Divider key="div" />,
];
Expand Down Expand Up @@ -495,6 +504,24 @@ export class DateRangePicker extends AbstractPureComponent<IDateRangePickerProps
this.handleNextState(nextValue);
};

private handleShortcutClick = (shortcut: IDateRangeShortcut) => {
const { dateRange, shouldChangeTime } = shortcut;
if (shouldChangeTime) {
const newDateRange: DateRange = [dateRange[0], dateRange[1]];
const newTimeRange: DateRange = [dateRange[0], dateRange[1]];
const nextState = getStateChange(
this.state.value,
dateRange,
this.state,
this.props.contiguousCalendarMonths,
);
this.setState({ ...nextState, time: newTimeRange });
Utils.safeInvoke(this.props.onChange, newDateRange);
} else {
this.handleNextState(dateRange);
}
};

private handleNextState = (nextValue: DateRange) => {
const { value } = this.state;
nextValue[0] = DateUtils.getDateTime(nextValue[0], this.state.time[0]);
Expand Down
9 changes: 5 additions & 4 deletions packages/datetime/src/shortcuts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ import { clone, DateRange, isDayRangeInRange } from "./common/dateUtils";
export interface IDateRangeShortcut {
label: string;
dateRange: DateRange;
shouldChangeTime?: boolean;
}

export interface IShortcutsProps {
allowSingleDayRange: boolean;
minDate: Date;
maxDate: Date;
shortcuts: IDateRangeShortcut[] | true;
onShortcutClick: (shortcut: DateRange) => void;
onShortcutClick: (shortcut: IDateRangeShortcut) => void;
}

export class Shortcuts extends React.PureComponent<IShortcutsProps> {
Expand All @@ -34,16 +35,16 @@ export class Shortcuts extends React.PureComponent<IShortcutsProps> {
className={Classes.POPOVER_DISMISS_OVERRIDE}
disabled={!this.isShortcutInRange(s.dateRange)}
key={i}
onClick={this.getShorcutClickHandler(s.dateRange)}
onClick={this.getShorcutClickHandler(s)}
text={s.label}
/>
));

return <Menu className={DATERANGEPICKER_SHORTCUTS}>{shortcutElements}</Menu>;
}

private getShorcutClickHandler(nextValue: DateRange) {
return () => this.props.onShortcutClick(nextValue);
private getShorcutClickHandler(shortcut: IDateRangeShortcut) {
return () => this.props.onShortcutClick(shortcut);
}

private isShortcutInRange(shortcutDateRange: DateRange) {
Expand Down
104 changes: 74 additions & 30 deletions packages/datetime/test/dateRangePickerTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,47 +904,74 @@ describe("<DateRangePicker>", () => {
});

it("custom shortcuts set the displayed months correctly when start month changes", () => {
const dateRange = [new Date(2016, Months.JANUARY, 1), new Date(2016, Months.DECEMBER, 31)] as DateRange;
const { left, right } = render({
initialMonth: new Date(2015, Months.JANUARY, 1),
shortcuts: [{ label: "custom shortcut", dateRange }],
}).clickShortcut();
assert.isTrue(onChangeSpy.calledOnce);
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);
});
const dateRange = [
new Date(2016, Months.JANUARY, 1, 10, 20, 30),
new Date(2016, Months.DECEMBER, 31, 10, 20, 30),
] as DateRange;

it(
"custom shortcuts set the displayed months correctly when start month changes " +
"and contiguousCalendarMonths is false",
() => {
const dateRange = [new Date(2016, Months.JANUARY, 1), new Date(2016, Months.DECEMBER, 31)] as DateRange;
const test = (shouldChangeTime: boolean) => {
const { left, right } = render({
contiguousCalendarMonths: false,
initialMonth: new Date(2015, Months.JANUARY, 1),
shortcuts: [{ label: "custom shortcut", dateRange }],
shortcuts: [{ label: "custom shortcut", dateRange, shouldChangeTime }],
}).clickShortcut();
assert.isTrue(onChangeSpy.calledOnce);
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.DECEMBER, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);
};

test(true);
test(false);
Copy link
Contributor

@giladgray giladgray Jan 15, 2019

Choose a reason for hiding this comment

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

❌ these should be separate it() tests. please refactor this code so the time-changing tests run separately.

honestly i'd prefer if you 1) reverted this entire file and then 2) simply added a single test with shouldChangeTime: true to validate that the prop does what we think.

});

it(
"custom shortcuts set the displayed months correctly when start month changes " +
"and contiguousCalendarMonths is false",
() => {
const dateRange = [
new Date(2016, Months.JANUARY, 1, 10, 20, 30),
new Date(2016, Months.DECEMBER, 31, 10, 20, 30),
] as DateRange;

const test = (shouldChangeTime: boolean) => {
const { left, right } = render({
contiguousCalendarMonths: false,
initialMonth: new Date(2015, Months.JANUARY, 1),
shortcuts: [{ label: "custom shortcut", dateRange, shouldChangeTime }],
}).clickShortcut();
assert.isTrue(onChangeSpy.calledOnce);
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.DECEMBER, 2016);
};

test(true);
test(false);
},
);

it("custom shortcuts set the displayed months correctly when start month stays the same", () => {
const dateRange = [new Date(2016, Months.JANUARY, 1), new Date(2016, Months.DECEMBER, 31)] as DateRange;
const { clickShortcut, left, right } = render({
initialMonth: new Date(2016, Months.JANUARY, 1),
shortcuts: [{ label: "custom shortcut", dateRange }],
});
const dateRange = [
new Date(2016, Months.JANUARY, 1, 10, 20, 30),
new Date(2016, Months.DECEMBER, 31, 10, 20, 30),
] as DateRange;

const test = (shouldChangeTime: boolean) => {
const { clickShortcut, left, right } = render({
initialMonth: new Date(2016, Months.JANUARY, 1),
shortcuts: [{ label: "custom shortcut", dateRange, shouldChangeTime }],
});

clickShortcut();
assert.isTrue(onChangeSpy.calledOnce);
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);
clickShortcut();
assert.isTrue(onChangeSpy.calledOnce);
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);

clickShortcut();
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);
clickShortcut();
left.assertMonthYear(Months.JANUARY, 2016);
right.assertMonthYear(Months.FEBRUARY, 2016);
};

test(true);
test(false);
});
});

Expand Down Expand Up @@ -1147,11 +1174,28 @@ describe("<DateRangePicker>", () => {
assert.isTrue(DateUtils.areSameDay(onChangeSpy.firstCall.args[0][0] as Date, new Date()));
});

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

it("clicking a shortcut with shouldChangeTime=true changes time", () => {
const endTime = defaultRange[1];
const startTime = new Date(defaultRange[1].getTime());
startTime.setHours(startTime.getHours() - 2);

const shortcuts = [
{
dateRange: [startTime, endTime] as DateRange,
label: "custom shortcut",
shouldChangeTime: true,
},
];

render({ timePrecision: "minute", defaultValue: defaultRange, shortcuts }).clickShortcut();
assert.equal(onChangeSpy.firstCall.args[0][0] as Date, startTime);
});

it("selecting and unselecting a day doesn't change time", () => {
const leftDatePicker = render({ timePrecision: "minute", defaultValue: defaultRange }).left;
leftDatePicker.clickDay(5);
Expand Down