-
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
Conversation
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.
thanks @mud! looks pretty good, reasonable approach.
docs-app preview from 031cfdf
packages/datetime/src/dateInput.tsx
Outdated
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), |
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.
why the timeout here? leave a code comment explaining why it's necessary
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.
also, should safeInvoke(this.props.dayPickerProps.onMonthChange)
in here, or suddenly that one prop is not supported.
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.
- add why timeout is required as comment
-
safeInvoke(this.props.dayPickerProps.onMonthChange)
packages/datetime/src/dateInput.tsx
Outdated
@@ -198,15 +200,29 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |||
}; | |||
} | |||
|
|||
public componentWillUnmount() { | |||
this.unregisterPopoverBlurHandler(); | |||
super.componentWillUnmount(); |
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
first
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 first
@@ -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 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
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.
-
use popover's close implementation, remove this test
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.
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.
packages/datetime/src/dateInput.tsx
Outdated
const node = ReactDOM.findDOMNode(this); | ||
const tabbableElements = node.querySelectorAll("[tabindex]:not([tabindex='-1'])"); | ||
const numOfElements = tabbableElements.length; | ||
if (numOfElements) { |
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.
avoid boolean coercion. tabbableElements.length > 0
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.
-
tabbableElements.length > 0
packages/datetime/src/dateInput.tsx
Outdated
} | ||
}; | ||
|
||
private handlePopoverBlur = () => { |
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.
no need for this method with optional arg in handleClosePopover
, just use handleClosePopover
directly
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.
- remove and call
handleClosePopover
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.
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?
@@ -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) { |
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.
popovers have a feature to closeOnEscapeKey
... can we leverage that instead?
try adding an onInteraction
handler to the Popover
.
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.
-
usecloseOnEscapeKey
and tryonInteraction
on the popover
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.
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.
packages/datetime/src/dateInput.tsx
Outdated
} | ||
this.safeInvokeInputProp("onKeyDown", e); | ||
}; | ||
|
||
private registerPopoverBlurHandler = () => { | ||
const node = ReactDOM.findDOMNode(this); |
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.
i'm not a huge fan of using findDOMNode
, tend to prefer refs. but this is alright.
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.
- maybe use refs?
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.
I just wrapped the popover content since ReactDayPicker.Props
doesn't have a prop to expose ref.
packages/datetime/src/dateInput.tsx
Outdated
} | ||
this.safeInvokeInputProp("onKeyDown", e); | ||
}; | ||
|
||
private registerPopoverBlurHandler = () => { | ||
const node = ReactDOM.findDOMNode(this); | ||
const tabbableElements = node.querySelectorAll("[tabindex]:not([tabindex='-1'])"); |
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.
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)
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.
- check to see if we can reasonably support other focusable components
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.
Actually, we will need to if we also want to support datetime.
Sweet, thanks for the CR @giladgray. Will work on your comments. |
packages/datetime/src/dateInput.tsx
Outdated
// for the updated month to be rendered | ||
onMonthChange: (month: Date) => | ||
setTimeout(() => { | ||
Utils.safeInvoke(this.props.dayPickerProps.onMonthChange, month); |
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.
but this shouldn't be in the timeout, it should be called synchronously.
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.
- move
safeInvoke
out ofsetTimeout
packages/datetime/src/dateInput.tsx
Outdated
onMonthChange: () => setTimeout(this.registerPopoverBlurHandler, 0), | ||
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
👌
packages/datetime/src/dateInput.tsx
Outdated
// onMonthChange is called. setTimeout is necessary to wait | ||
// for the updated month to be rendered | ||
onMonthChange: (month: Date) => | ||
setTimeout(() => { |
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.
please use this.setTimeout
instead, and remove unnecessary 0
arg. see https://github.com/palantir/blueprint/blob/develop/packages/core/src/common/abstractComponent.ts#L43
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.
- use
this.setTimeout
remove0
@@ -232,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>; |
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.
another wrapper element 😢
guess that's how it is.
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.
- acknowledge sadness 😿
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.
😂 feeling better already
packages/datetime/src/dateInput.tsx
Outdated
this.unregisterPopoverBlurHandler(); | ||
this.lastPopoverElement = lastPopoverElement; | ||
this.lastPopoverElement.addEventListener("blur", this.handlePopoverBlur); | ||
if (this.contentRef) { |
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.
avoid boolean coercion. != null
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.
-
this.contentRef != null
packages/datetime/src/dateInput.tsx
Outdated
} | ||
} | ||
}; | ||
|
||
private unregisterPopoverBlurHandler = () => { | ||
if (this.lastPopoverElement) { | ||
this.lastPopoverElement.removeEventListener("blur", this.handlePopoverBlur); | ||
this.lastPopoverElement.removeEventListener("blur", (this.handleClosePopover as any) as EventListener); |
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.
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();
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.
- use snippet above
packages/datetime/src/dateInput.tsx
Outdated
}, 0), | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
@@ -232,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>; |
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.
😂 feeling better already
one min -- taking a look now |
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 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
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.
this is already updated. queryAll
was removed.
packages/datetime/src/dateInput.tsx
Outdated
const tabbableElements = this.contentRef.querySelectorAll("input, [tabindex]:not([tabindex='-1'])"); | ||
const numOfElements = tabbableElements.length; | ||
if (numOfElements > 0) { | ||
const lastPopoverElement = tabbableElements[numOfElements - 1] as HTMLElement; |
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.
why do we need to do all this (keep track of the last popover element and juggle the event handlers)? please leave comments
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.
at first I thought this was the last popover element on the page, but it's actually more like lastElementInPopover
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.
- Add comment on why we keep track of
lastPopoverElement
- rename
lastPopoverElement
->lastElementInPopover
packages/datetime/src/dateInput.tsx
Outdated
}; | ||
|
||
private unregisterPopoverBlurHandler = () => { | ||
if (this.lastPopoverElement) { |
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.
if (this.lastPopoverElement != null)
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.
- fix
packages/datetime/src/dateInput.tsx
Outdated
if (this.lastPopoverElement !== lastPopoverElement) { | ||
this.unregisterPopoverBlurHandler(); | ||
this.lastPopoverElement = lastPopoverElement; | ||
this.lastPopoverElement.addEventListener("blur", this.handlePopoverBlur); |
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.
is this right? what if you TAB to the last element and then SHIFT+TAB to go backwards?
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.
Actually, if you SHIFT+TAB on the last element, it will close. Will fix.
- Handle SHIFT+TAB and make sure it focuses previous element.
sorry, I meant to hint at the more general case -- blurring the last tabbable element in the popover can happen in a number of ways, and only one of them should close the popover through this new code you're adding (TAB forward). another case we should make sure to test is focussing that |
packages/datetime/src/dateInput.tsx
Outdated
// 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 comment
The 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).
packages/datetime/src/dateInput.tsx
Outdated
// 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 comment
The 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.
…event. Using relatedTarget in focus event to see if focus moved out of popover or not.
@giladgray @adidahiya can you take a look and see if we can get this merged? |
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.
👍 looks good! sorry @mud, we're all focused on final 2.0 work
np. good luck with 2.0! |
Changes proposed in this pull request:
This PR enables the dateinput popover to be closed:
Currently, shift tabbing out of the dateinput will close the date popover, however, tabbing through the popover elements do not close the popover after tabbing out of the last tabbable element. ESC also does not have any effect.
Reviewers should focus on:
Is there a better way to implement this feature?
Screenshot