-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 8 commits
7f20c79
85bb3d5
031cfdf
4f49526
e6b3f41
9fd606d
76253fe
dafca2c
8745263
69dd5ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 lastElementInPopover: HTMLElement = null; | ||
|
||
public constructor(props?: IDateInputProps, context?: any) { | ||
super(props, context); | ||
|
@@ -198,15 +200,35 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |
}; | ||
} | ||
|
||
public componentWillUnmount() { | ||
super.componentWillUnmount(); | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 |
||
onMonthChange: (month: Date) => { | ||
Utils.safeInvoke(this.props.dayPickerProps.onMonthChange, month); | ||
this.setTimeout(this.registerPopoverBlurHandler); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 |
||
}, | ||
}; | ||
|
||
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} | ||
|
@@ -216,6 +238,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |
timePickerProps={{ ...this.props.timePickerProps, precision: this.props.timePrecision }} | ||
/> | ||
); | ||
const wrappedPopoverContent = <div ref={this.setContentRef}>{popoverContent}</div>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another wrapper element 😢 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -239,7 +262,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)} | ||
|
@@ -307,7 +330,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 }); | ||
|
@@ -434,6 +457,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |
this.setState({ isInputFocused: false }); | ||
} | ||
} | ||
this.registerPopoverBlurHandler(); | ||
this.safeInvokeInputProp("onBlur", e); | ||
}; | ||
|
||
|
@@ -447,16 +471,58 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. popovers have a feature to try adding an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}; | ||
|
||
// keyboard DOM event listener (not React event) | ||
private handlePopoverBlur = (e: KeyboardEvent) => { | ||
if (e.which === Keys.TAB && !e.shiftKey) { | ||
e.target.dispatchEvent(new FocusEvent("blur")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? this is suspicious. we're not in the habit of dispatching DOM events from blueprint code (except where absolutely necessary in tests). |
||
this.handleClosePopover(); | ||
} | ||
}; | ||
|
||
private registerPopoverBlurHandler = () => { | ||
if (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'])"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is already updated. |
||
const numOfElements = tabbableElements.length; | ||
if (numOfElements > 0) { | ||
// Keep track of the last focusable element in popover and add | ||
// a keydown handler, so that: | ||
// * popover closes when the user tabs to the next element | ||
// * or focus moves to previous element if shift+tab | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something is wrong with the language in this comment. it doesn't parse correctly. i think "popover closes:" belongs after "so that", but then the second bullet sounds incorrect. |
||
const lastElement = tabbableElements[numOfElements - 1] as HTMLElement; | ||
if (this.lastElementInPopover !== lastElement) { | ||
this.unregisterPopoverBlurHandler(); | ||
this.lastElementInPopover = lastElement; | ||
this.lastElementInPopover.addEventListener("keydown", this.handlePopoverBlur); | ||
} | ||
} | ||
} | ||
}; | ||
|
||
private unregisterPopoverBlurHandler = () => { | ||
if (this.lastElementInPopover != null) { | ||
this.lastElementInPopover.removeEventListener("keydown", this.handlePopoverBlur); | ||
} | ||
}; | ||
|
||
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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,31 @@ describe("<DateInput>", () => { | |
assert.isFalse(wrapper.find(Popover).prop("isOpen")); | ||
}); | ||
|
||
it("Popover closes when ESC key pressed", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call
super
firstThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.