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
61 changes: 58 additions & 3 deletions packages/datetime/src/dateInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
public static displayName = "Blueprint.DateInput";

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

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

public componentWillUnmount() {
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

this.unregisterPopoverBlurHandler();
}

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,
// dom elements for the updated month is not available when
// onMonthChange is called. setTimeout is necessary to wait
// for the updated month to be rendered
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

onMonthChange: (month: Date) =>
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 this.setTimeout remove 0

Utils.safeInvoke(this.props.dayPickerProps.onMonthChange, month);
Copy link
Contributor

Choose a reason for hiding this comment

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

but this shouldn't be in the timeout, it should be called synchronously.

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.

  • move safeInvoke out of setTimeout

this.registerPopoverBlurHandler();
}, 0),
};

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 All @@ -216,6 +239,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
timePickerProps={{ ...this.props.timePickerProps, precision: this.props.timePrecision }}
/>
);
const wrappedPopoverContent = <div ref={this.setContentRef}>{popoverContent}</div>;
Copy link
Contributor

Choose a reason for hiding this comment

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

another wrapper element 😢
guess that's how it is.

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.

  • acknowledge sadness 😿

Copy link
Contributor

Choose a reason for hiding this comment

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

😂 feeling better already

// assign default empty object here to prevent mutation
const { inputProps = {}, popoverProps = {}, format } = this.props;
// exclude ref (comes from HTMLInputProps typings, not InputGroup)
Expand All @@ -239,7 +263,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
{...popoverProps}
autoFocus={false}
className={popoverClassName}
content={popoverContent}
content={wrappedPopoverContent}
enforceFocus={false}
onClose={this.handleClosePopover}
popoverClassName={classNames("pt-dateinput-popover", popoverProps.popoverClassName)}
Expand Down Expand Up @@ -307,7 +331,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 +458,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat
this.setState({ isInputFocused: false });
}
}
this.registerPopoverBlurHandler();
this.safeInvokeInputProp("onBlur", e);
};

Expand All @@ -447,16 +472,46 @@ 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 = () => {
if (this.contentRef) {
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. != null

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.

  • this.contentRef != null

// Popover contents are well structured, but the selector will need
// to be updated if more focusable components are added in the future
const tabbableElements = this.contentRef.querySelectorAll("input, [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.

note to self / @giladgray: we'll need to update this for dom4 v2

Copy link
Contributor

Choose a reason for hiding this comment

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

this is already updated. queryAll was removed.

const numOfElements = tabbableElements.length;
if (numOfElements > 0) {
const lastPopoverElement = tabbableElements[numOfElements - 1] as HTMLElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to do all this (keep track of the last popover element and juggle the event handlers)? please leave comments

Copy link
Contributor

Choose a reason for hiding this comment

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

at first I thought this was the last popover element on the page, but it's actually more like lastElementInPopover

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.

  • Add comment on why we keep track of lastPopoverElement
  • rename lastPopoverElement -> lastElementInPopover

if (this.lastPopoverElement !== lastPopoverElement) {
this.unregisterPopoverBlurHandler();
this.lastPopoverElement = lastPopoverElement;
this.lastPopoverElement.addEventListener("blur", (this.handleClosePopover as any) as EventListener);
}
}
}
};

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.handleClosePopover as any) as EventListener);
Copy link
Contributor

Choose a reason for hiding this comment

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

oh whoa well if the alternative is an as any then just do the handlePopoverBlur, but put it all on one line with a comment.

// blur DOM event listener (not React event)
private handlePopoverBlur = () => this.handleClosePopover();

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 snippet above

}
};

private setInputRef = (el: HTMLElement) => {
this.inputRef = el;
const { inputProps = {} } = this.props;
Utils.safeInvoke(inputProps.inputRef, el);
};

private setContentRef = (el: HTMLElement) => {
this.contentRef = el;
};

/** safe wrapper around invoking input props event handler (prop defaults to undefined) */
private safeInvokeInputProp(name: keyof HTMLInputProps, e: React.SyntheticEvent<HTMLElement>) {
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