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.

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

Jump to bottom

Fix DateRangeInput3 focus with timePrecision #6881

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Fix DateRangeInput3 focus with timePrecision #6881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec0666b
Fix DateRangeInput3 focus with timePrecision
evansjohnson Jun 27, 2024
d22fe85
lint and format
evansjohnson Jun 28, 2024
ec45c07
dont early return before checking timePrecision...
evansjohnson Jun 28, 2024
410431c
diff
evansjohnson Jun 28, 2024
dbfc8e6
fix tests
evansjohnson Jun 28, 2024
88c0f45
add more tests and fix more tests
evansjohnson Jun 28, 2024
9b6f3f0
format lint
evansjohnson Jun 28, 2024
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
49 changes: 37 additions & 12 deletions packages/datetime2/src/components/date-range-input3/dateRangeInput3.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ import {
DateUtils,
Errors,
type NonNullDateRange,
TimePicker,
type TimePrecision,
} from "@blueprintjs/datetime";

import { Classes } from "../../classes";
Expand Down Expand Up @@ -288,6 +290,18 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
// Callbacks - DateRangePicker3
// ===========================

private getTimePrecision = () => {
// timePrecision may be set as a root prop or as a property inside timePickerProps, so we need to check both
const { timePickerProps, timePrecision } = this.props;
const fromProps = timePickerProps?.precision ?? timePrecision;
if (fromProps != null) {
return fromProps;
}

// if timePickerProps is defined, the `TimePicker` will render with it's default `precision`
return timePickerProps != null ? TimePicker.defaultProps.precision : undefined;
Comment on lines +301 to +302
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confused by the comment above. Will the TimePicker still render with its default precision if timePickerProps is undefined?

I'm curious how important it is to return TimePicker.defaultProps.precision vs undefined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of this bit of existing behavior, which I tried to explain more here: https://github.com/palantir/blueprint/pull/6881/files#diff-d2a31417b184106fdf4054399ab49455504ff747c19714aa6df0bb7f8e26bf90R238-R239

let me know any suggestions you can think of to help clarify this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if timePickerProps is not defined, the underlying DateRangePicker will have default timePickerProps, and therefor not show the time pickers - so in that case we say the precision is `undefined.

if timePickerProps is defined, we return the default precision, since at this point in this method we already would have returned an explicitly defined precision either directly on this component, or from the timePickerProps

};

private handleDateRangePickerChange = (selectedRange: DateRange, didSubmitWithEnter = false) => {
// ignore mouse events in the date-range picker if the popover is animating closed.
if (!this.state.isOpen) {
Expand All @@ -306,9 +320,11 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr

let boundaryToModify: Boundary | undefined;

const timePrecision = this.getTimePrecision();

if (selectedStart == null) {
// focus the start field by default or if only an end date is specified
if (this.props.timePrecision == null) {
if (timePrecision == null) {
isStartInputFocused = true;
isEndInputFocused = false;
} else {
Expand All @@ -321,7 +337,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
startHoverString = null;
} else if (selectedEnd == null) {
// focus the end field if a start date is specified
if (this.props.timePrecision == null) {
if (timePrecision == null) {
isStartInputFocused = false;
isEndInputFocused = true;
} else {
Expand All @@ -335,7 +351,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
isOpen = this.getIsOpenValueWhenDateChanges(selectedStart, selectedEnd);
isStartInputFocused = false;

if (this.props.timePrecision == null && didSubmitWithEnter) {
if (timePrecision == null && didSubmitWithEnter) {
// if we submit via click or Tab, the focus will have moved already.
// it we submit with Enter, the focus won't have moved, and setting
// the flag to false won't have an effect anyway, so leave it true.
Expand All @@ -346,15 +362,15 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
}
} else if (this.state.lastFocusedField === Boundary.START) {
// keep the start field focused
if (this.props.timePrecision == null) {
if (timePrecision == null) {
isStartInputFocused = true;
isEndInputFocused = false;
} else {
isStartInputFocused = false;
isEndInputFocused = false;
boundaryToModify = Boundary.START;
}
} else if (this.props.timePrecision == null) {
} else if (timePrecision == null) {
// keep the end field focused
isStartInputFocused = false;
isEndInputFocused = true;
Expand Down Expand Up @@ -560,6 +576,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
isValueControlled ? values.controlledValue : values.selectedValue,
this.props,
this.state.locale,
this.getTimePrecision(),
true,
);

Expand Down Expand Up @@ -593,7 +610,12 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
if (isValueControlled) {
nextState = {
...nextState,
[keys.inputString]: formatDateString(values.controlledValue, this.props, this.state.locale),
[keys.inputString]: formatDateString(
values.controlledValue,
this.props,
this.state.locale,
this.getTimePrecision(),
),
};
} else {
nextState = {
Expand Down Expand Up @@ -680,7 +702,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
private getIsOpenValueWhenDateChanges = (nextSelectedStart: Date, nextSelectedEnd: Date): boolean => {
if (this.props.closeOnSelection) {
// trivial case when TimePicker is not shown
if (this.props.timePrecision == null) {
if (this.getTimePrecision() == null) {
return false;
}

Expand Down Expand Up @@ -747,7 +769,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
} else if (this.doesEndBoundaryOverlapStartBoundary(selectedValue, boundary)) {
return this.props.overlappingDatesMessage;
} else {
return formatDateString(selectedValue, this.props, this.state.locale);
return formatDateString(selectedValue, this.props, this.state.locale, this.getTimePrecision());
}
};

Expand Down Expand Up @@ -893,7 +915,7 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr
// N.B. this.state will be undefined in the constructor, so we need a fallback in that case
const maybeLocale = this.state?.locale ?? typeof props.locale === "string" ? undefined : props.locale;

return formatDateString(date ?? defaultDate, this.props, maybeLocale);
return formatDateString(date ?? defaultDate, this.props, maybeLocale, this.getTimePrecision());
};

private parseDate = (dateString: string | undefined): Date | null => {
Expand All @@ -907,7 +929,8 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr

// HACKHACK: this code below is largely copied from the `useDateParser()` hook, which is the preferred
// implementation that we can migrate to once DateRangeInput3 is a function component.
const { dateFnsFormat, locale: localeFromProps, parseDate, timePickerProps, timePrecision } = this.props;
const { dateFnsFormat, locale: localeFromProps, parseDate, timePickerProps } = this.props;
const timePrecision = this.getTimePrecision();
const { locale } = this.state;
let newDate: false | Date | null = null;

Expand All @@ -931,7 +954,8 @@ export class DateRangeInput3 extends DateFnsLocalizedComponent<DateRangeInput3Pr

// HACKHACK: the code below is largely copied from the `useDateFormatter()` hook, which is the preferred
// implementation that we can migrate to once DateRangeInput3 is a function component.
const { dateFnsFormat, formatDate, locale: localeFromProps, timePickerProps, timePrecision } = this.props;
const { dateFnsFormat, formatDate, locale: localeFromProps, timePickerProps } = this.props;
const timePrecision = this.getTimePrecision();
const { locale } = this.state;

if (formatDate !== undefined) {
Expand All @@ -950,6 +974,7 @@ function formatDateString(
date: Date | false | null | undefined,
props: DateRangeInput3Props,
locale: Locale | undefined,
timePrecision: TimePrecision | undefined,
ignoreRange = false,
) {
const { invalidDateMessage, maxDate, minDate, outOfRangeMessage } = props as DateRangeInput3PropsWithDefaults;
Expand All @@ -961,7 +986,7 @@ function formatDateString(
} else if (ignoreRange || DateUtils.isDayInRange(date, [minDate, maxDate])) {
// HACKHACK: the code below is largely copied from the `useDateFormatter()` hook, which is the preferred
// implementation that we can migrate to once DateRangeInput3 is a function component.
const { dateFnsFormat, formatDate, locale: localeFromProps, timePickerProps, timePrecision } = props;
const { dateFnsFormat, formatDate, locale: localeFromProps, timePickerProps } = props;
if (formatDate !== undefined) {
// user-provided date formatter
return formatDate(date, locale?.code ?? getLocaleCodeFromProps(localeFromProps));
Expand Down
16 changes: 12 additions & 4 deletions packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export class DateRangePicker3 extends DateFnsLocalizedComponent<DateRangePicker3
}

const { selectedShortcutIndex } = this.state;
const { allowSingleDayRange, maxDate, minDate, timePrecision } = this.props;
const { allowSingleDayRange, maxDate, minDate } = this.props;
return [
<DatePickerShortcutMenu
key="shortcuts"
Expand All @@ -218,17 +218,25 @@ export class DateRangePicker3 extends DateFnsLocalizedComponent<DateRangePicker3
minDate,
selectedShortcutIndex,
shortcuts,
timePrecision,
timePrecision: this.getTimePrecision(),
}}
onShortcutClick={this.handleShortcutClick}
/>,
<Divider key="div" />,
];
}

private maybeRenderTimePickers(isShowingOneMonth: boolean) {
private getTimePrecision = () => {
// timePrecision may be set as a root prop or as a property inside timePickerProps, so we need to check both
const { timePickerProps, timePrecision = timePickerProps?.precision } = this.props;
const { timePickerProps, timePrecision } = this.props;
return timePickerProps?.precision ?? timePrecision;
};

private maybeRenderTimePickers(isShowingOneMonth: boolean) {
const { timePickerProps } = this.props;
const timePrecision = this.getTimePrecision();
// setting empty object `timePickerProps` is enough to get the time picker to render with default props
// as defined in the TimePicker
if (timePrecision == null && timePickerProps === DateRangePicker3.defaultProps.timePickerProps) {
return null;
}
Expand Down
69 changes: 39 additions & 30 deletions packages/datetime2/test/components/dateRangeInput3Tests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,45 +177,54 @@ describe("<DateRangeInput3>", () => {
expectPropValidationError(DateRangeInput3, { ...DATE_FORMAT, value: null! });
});

describe("timePrecision prop", () => {
it("<TimePicker /> should not lose focus on increment/decrement with up/down arrows", () => {
const { root } = wrap(<DateRangeInput3 {...DATE_FORMAT} timePrecision={TimePrecision.MINUTE} />, true);
describe("<TimePicker /> focus", () => {
// there are multiple ways to render the underlying TimePicker component so run same tests on all
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice coverage of tests! I like that we're checking all these variants.

const toTest = [
{ label: "timePrecision != null", props: { timePrecision: TimePrecision.MINUTE } },
{
label: "timePickerProps.precision != null",
props: { timePickerProps: { precision: TimePrecision.MINUTE } },
},
{ label: "timePickerProps is {}", props: { timePickerProps: {} } },
];

root.setState({ isOpen: true }).update();
expect(root.find(Popover).prop("isOpen"), "Popover isOpen").to.be.true;
toTest.forEach(({ label, props }) => {
it(`when ${label}, <TimePicker /> should not lose focus on increment/decrement with up/down arrows`, () => {
const { root } = wrap(<DateRangeInput3 {...DATE_FORMAT} {...props} />, true);

keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
expect(isStartInputFocused(root), "start input is focused").to.be.false;
expect(isEndInputFocused(root), "end input is focused").to.be.false;
});
root.setState({ isOpen: true }).update();
expect(root.find(Popover).prop("isOpen"), "Popover isOpen").to.be.true;

it("when timePrecision != null && closeOnSelection=true && <TimePicker /> values is changed popover should not close", () => {
const { root, getDayElement } = wrap(
<DateRangeInput3 {...DATE_FORMAT} timePrecision={TimePrecision.MINUTE} />,
true,
);
keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
expect(isStartInputFocused(root), "start input is focused").to.be.false;
expect(isEndInputFocused(root), "end input is focused").to.be.false;
});

root.setState({ isOpen: true }).update();
it(`when ${label} && closeOnSelection=true && <TimePicker /> values is changed popover should not close`, () => {
const { root, getDayElement } = wrap(<DateRangeInput3 {...DATE_FORMAT} {...props} />, true);

getDayElement(1).simulate("click");
getDayElement(10).simulate("click");
root.setState({ isOpen: true }).update();

root.setState({ isOpen: true }).update();
getDayElement(1).simulate("click");
getDayElement(10).simulate("click");

keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
root.update();
expect(root.find(Popover).prop("isOpen")).to.be.true;
});
root.setState({ isOpen: true }).update();

keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
root.update();
expect(root.find(Popover).prop("isOpen")).to.be.true;
});

it("when timePrecision != null && closeOnSelection=true && end <TimePicker /> values is changed directly (without setting the selectedEnd date) - popover should not close", () => {
const { root } = wrap(<DateRangeInput3 {...DATE_FORMAT} timePrecision={TimePrecision.MINUTE} />, true);
it(`when ${label} && closeOnSelection=true && end <TimePicker /> values is changed directly (without setting the selectedEnd date) - popover should not close`, () => {
const { root } = wrap(<DateRangeInput3 {...DATE_FORMAT} {...props} />, true);

root.setState({ isOpen: true });
keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
root.update();
keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp", 1);
root.update();
expect(root.find(Popover).prop("isOpen")).to.be.true;
root.setState({ isOpen: true });
keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp");
root.update();
keyDownOnInput(DatetimeClasses.TIMEPICKER_HOUR, "ArrowUp", 1);
root.update();
expect(root.find(Popover).prop("isOpen")).to.be.true;
});
});

function keyDownOnInput(className: string, key: string, inputElementIndex: number = 0) {
Expand Down