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

[release/1.x] Close dateinput popover #2093

Merged
merged 10 commits into from
Feb 15, 2018
48 changes: 46 additions & 2 deletions packages/datetime/src/dateInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as classNames from "classnames";
import * as moment from "moment";
import * as React from "react";
import * as ReactDayPicker from "react-day-picker";
import * as ReactDOM from "react-dom";

import {
AbstractComponent,
Expand Down Expand Up @@ -184,6 +185,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
public static displayName = "Blueprint.DateInput";

private inputRef: HTMLElement = null;
private lastPopoverElement: HTMLElement = null;

public constructor(props?: IDateInputProps, context?: any) {
super(props, context);
Expand All @@ -198,15 +200,29 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
};
}

public componentWillUnmount() {
this.unregisterPopoverBlurHandler();
super.componentWillUnmount();
Copy link
Contributor

Choose a reason for hiding this comment

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

call super first

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • call super first

}

public render() {
const { value, valueString } = this.state;
const dateString = this.state.isInputFocused ? valueString : this.getDateString(value);
const date = this.state.isInputFocused ? this.createMoment(valueString) : value;
const dateValue = this.isMomentValidAndInRange(value) ? fromMomentToDate(value) : null;
const dayPickerProps = {
...this.props.dayPickerProps,
onMonthChange: () => setTimeout(this.registerPopoverBlurHandler, 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

why the timeout here? leave a code comment explaining why it's necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

also, should safeInvoke(this.props.dayPickerProps.onMonthChange) in here, or suddenly that one prop is not supported.

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • add why timeout is required as comment
  • safeInvoke(this.props.dayPickerProps.onMonthChange)

};

const popoverContent =
this.props.timePrecision === undefined ? (
<DatePicker {...this.props} onChange={this.handleDateChange} value={dateValue} />
<DatePicker
{...this.props}
dayPickerProps={dayPickerProps}
onChange={this.handleDateChange}
value={dateValue}
/>
) : (
<DateTimePicker
canClearSelection={this.props.canClearSelection}
Expand Down Expand Up @@ -307,7 +323,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
return isMomentInRange(value, this.props.minDate, this.props.maxDate);
}

private handleClosePopover = (e: React.SyntheticEvent<HTMLElement>) => {
private handleClosePopover = (e?: React.SyntheticEvent<HTMLElement>) => {
const { popoverProps = {} } = this.props;
Utils.safeInvoke(popoverProps.onClose, e);
this.setState({ isOpen: false });
Expand Down Expand Up @@ -434,6 +450,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
this.setState({ isInputFocused: false });
}
}
this.registerPopoverBlurHandler();
this.safeInvokeInputProp("onBlur", e);
};

Expand All @@ -447,10 +464,37 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
// the page. tabbing forward should *not* close the popover, because
// focus will be moving into the popover itself.
this.setState({ isOpen: false });
} else if (e.which === Keys.ESCAPE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

popovers have a feature to closeOnEscapeKey... can we leverage that instead?

try adding an onInteraction handler to the Popover.

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • use closeOnEscapeKey and try onInteraction on the popover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like without additional work, pressing ESC while focus is within the popover, the popover will close.

However, when the focus is on the input we still need to handle ESC and set the popover to close.

this.setState({ isOpen: false });
this.inputRef.blur();
}
this.safeInvokeInputProp("onKeyDown", e);
};

private registerPopoverBlurHandler = () => {
const node = ReactDOM.findDOMNode(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not a huge fan of using findDOMNode, tend to prefer refs. but this is alright.

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • maybe use refs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wrapped the popover content since ReactDayPicker.Props doesn't have a prop to expose ref.

const tabbableElements = node.querySelectorAll("[tabindex]:not([tabindex='-1'])");
Copy link
Contributor

Choose a reason for hiding this comment

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

any sense in supporting other focusable things like <button> or are we safe cuz the calendar contents are well-defined and only use tabIndex? (hoping we're safe... this is a rabbit hole)

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • check to see if we can reasonably support other focusable components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we will need to if we also want to support datetime.

const numOfElements = tabbableElements.length;
if (numOfElements) {
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid boolean coercion. tabbableElements.length > 0

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • tabbableElements.length > 0

const lastPopoverElement = tabbableElements[numOfElements - 1] as HTMLElement;
if (this.lastPopoverElement !== lastPopoverElement) {
this.unregisterPopoverBlurHandler();
this.lastPopoverElement = lastPopoverElement;
this.lastPopoverElement.addEventListener("blur", this.handlePopoverBlur);
}
}
};

private unregisterPopoverBlurHandler = () => {
if (this.lastPopoverElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (this.lastPopoverElement != null)

Copy link
Contributor Author

@mud mud Feb 7, 2018

Choose a reason for hiding this comment

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

  • fix

this.lastPopoverElement.removeEventListener("blur", this.handlePopoverBlur);
}
};

private handlePopoverBlur = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this method with optional arg in handleClosePopover, just use handleClosePopover directly

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • remove and call handleClosePopover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do something like: (this.handleClosePopover as any) as EventListener which doesn't look great. Is removing the extra method still more preferable?

this.handleClosePopover();
};

private setInputRef = (el: HTMLElement) => {
this.inputRef = el;
const { inputProps = {} } = this.props;
Expand Down
25 changes: 25 additions & 0 deletions packages/datetime/test/dateInputTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,31 @@ describe("<DateInput>", () => {
assert.isFalse(wrapper.find(Popover).prop("isOpen"));
});

it("Popover closes when ESC key pressed", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Popover provides this feature, and has corresponding tests. can we leverage it instead of reimplementing/testing here?
https://github.com/palantir/blueprint/blob/develop/packages/core/test/popover/popoverTests.tsx#L565

Copy link
Contributor Author

@mud mud Feb 6, 2018

Choose a reason for hiding this comment

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

  • use popover's close implementation, remove this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned up top, we still need the functionality for hitting ESC while the input is in focus. So I still think we need this.

const wrapper = mount(<DateInput openOnFocus={true} />);
const input = wrapper.find("input");
input.simulate("focus");
const popover = wrapper.find(Popover);
assert.isTrue(popover.prop("isOpen"));
input.simulate("keydown", { which: Keys.ESCAPE });
assert.isFalse(popover.prop("isOpen"));
});

it("Popover closes when last tabbable component is blurred", () => {
const wrapper = mount(<DateInput openOnFocus={true} />);
const input = wrapper.find("input");
input.simulate("focus");
const popover = wrapper.find(Popover);
assert.isTrue(popover.prop("isOpen"));
input.simulate("blur");
const lastTabbable = popover
.find(".DayPicker-Day--outside")
.last()
.getDOMNode() as HTMLElement;
lastTabbable.dispatchEvent(new Event("blur"));
assert.isFalse(popover.prop("isOpen"));
});

it("setting timePrecision renders a TimePicker", () => {
const wrapper = mount(<DateInput timePrecision={TimePickerPrecision.SECOND} />).setState({ isOpen: true });
// assert TimePicker appears
Expand Down