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
[Popover2] Make sure targetElement exists before trying to call its function #1857
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 for this! Nothing blocking, just one thing to fix before merging.
@@ -383,7 +383,7 @@ export class Popover2 extends AbstractComponent<IPopover2Props, IPopover2State> | |||
|
|||
private updateDarkParent() { | |||
if (!this.props.inline && this.state.isOpen) { | |||
const hasDarkParent = this.targetElement.closest(`.${Classes.DARK}`) != null; | |||
const hasDarkParent = this.targetElement && this.targetElement.closest(`.${Classes.DARK}`) != 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.
Codebase convention: explicit boolean expression please:
this.targetElement != null && ...
Explicit null checkPreview: documentation | landing | table |
@@ -383,7 +383,7 @@ export class Popover2 extends AbstractComponent<IPopover2Props, IPopover2State> | |||
|
|||
private updateDarkParent() { | |||
if (!this.props.inline && this.state.isOpen) { | |||
const hasDarkParent = this.targetElement && this.targetElement.closest(`.${Classes.DARK}`) != null; | |||
const hasDarkParent = this.targetElement !== null && this.targetElement.closest(`.${Classes.DARK}`) != 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.
Oh, you'll want to do double-equals (!=
) so that both undefined
and null
satisfy the condition.
Explicit null check, take two. action!Preview: documentation | landing | table |
const hasDarkParent = this.targetElement.closest(`.${Classes.DARK}`) != null; | ||
const hasDarkParent = | ||
this.targetElement != null && | ||
typeof this.targetElement !== "undefined" && |
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.
Line 387 is sufficient without this additional typeof check.
Explicit null check, take threePreview: documentation | landing | table |
Fixes #1856
Checklist
Changes proposed in this pull request:
Make sure targetElement exists before trying to call
closest
on it