-
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
rewrite HotkeysTarget and ContextMenuTarget as HOCs #1914
Conversation
fix lint and testsPreview: documentation | landing | table |
fix prettier lintPreview: documentation | landing | table |
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.
woooo this is 🔥
* Licensed under the terms of the LICENSE file distributed with this project. | ||
*/ | ||
|
||
export interface IConstructor<T> { |
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.
can we get a /**
explaining what/why
@@ -17,6 +17,8 @@ export { IKeyCombo, comboMatches, getKeyCombo, getKeyComboString, parseKeyCombo | |||
export { IHotkeysDialogProps, hideHotkeysDialog, setHotkeysDialogProps } from "./hotkeysDialog"; | |||
|
|||
export interface IHotkeysProps extends IProps { | |||
children?: React.ReactNode; |
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, please remove.
componentDidMount.call(this); | ||
public componentWillMount() { | ||
if (super.componentWillMount != null) { | ||
super.componentWillMount(); |
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.
safeInvoke
?
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.
mmm not a huge fan since you'll have to do safeInvoke(super.componentWillMount.bind(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.
hmm sure that's a good reason. maybe leave a code comment about "preserving this
binding" ?
const TargettedTestComponent = HotkeysTarget(TestComponent); | ||
|
||
// it's not the same Component | ||
expect(TargettedTestComponent).to.not.equal(TestComponent); |
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 expect...equal
the same as assert.strictEqual
?
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.
Yeah equal
is ===
return element; | ||
} | ||
public render() { | ||
// TODO handle being applied on a Component that doesn't return an actual Element |
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 code handles null | undefined | JSX.Element
. I think we should throw an error if element
is not one of those types (namely, string | number | Fragment
)
"@ContextMenuTarget-decorated components must return a single JSX.Element or an empty render."
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 it possible to support React.ReactNode
return types? that would include React.ReactPortal
.
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.
string/number/portal/fragment don't have real DOM elements so there's no place to put the onKeyDown/onKeyUp handlers (unless we add our own wrapper <div>
but I'm pretty opposed to this DOM monkeypatching)
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.
ok fair enough
packages/core/src/common/utils.ts
Outdated
@@ -22,6 +22,14 @@ export function isFunction(value: any): value is Function { | |||
return typeof value === "function"; | |||
} | |||
|
|||
export interface IHasName { |
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.
what's the use case for this? if we keep it, it should be called INamed
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 adapting https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging . The .name
is referencing Function.name
(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name) which evaluates to the class name for constructors which is what we want 90% of the time, but for some reason Typescript types don't include .name
as a field in Function
. This was the easiest way to express the type.
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.
cool, makes sense, can you leave a comment explaining that?
return element; | ||
} | ||
public render() { | ||
// TODO handle being applied on a Component that doesn't return an actual Element |
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 it possible to support React.ReactNode
return types? that would include React.ReactPortal
.
const onContextMenu = (e: React.MouseEvent<HTMLElement>) => { | ||
// support nested menus (inner menu target would have called preventDefault()) | ||
if (e.defaultPrevented) { | ||
return; |
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.
should we invoke oldOnContextMenu
in this code path?
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.
Not sure about the logic here; @giladgray ?
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 sure either. could just change next condition to be if (isFunction && !prevented)
.
throw new Error(`@HotkeysTarget-decorated class must implement \`renderHotkeys\`. ${constructor}`); | ||
export function HotkeysTarget<T extends IConstructor<IHotkeysTarget>>(WrappedComponent: T) { | ||
if (!isFunction(WrappedComponent.prototype.renderHotkeys)) { | ||
throw new Error(`@HotkeysTarget-decorated class must implement \`renderHotkeys\`. ${WrappedComponent}`); |
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 this to errors.ts
please
move warning messages to errors.ts
gracefully handle bad render casesPreview: documentation | landing | table |
IHasName -> INamedPreview: documentation | landing | table |
Fixes #931
Changes proposed in this pull request:
Implement
HotkeysTarget
andContextMenuTarget
by returning a new HOC rather than monkeypatching.Reviewers should focus on:
Behavior is maintained from before.