rewrite HotkeysTarget and ContextMenuTarget as HOCs #1914
Conversation
fix lint and testsPreview: documentation | landing | table |
fix prettier lintPreview: documentation | landing | table |
| * Licensed under the terms of the LICENSE file distributed with this project. | ||
| */ | ||
|
|
||
| export interface IConstructor<T> { |
There was a problem hiding this comment.
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.
🙅♂️ no need for this, please remove.
| componentDidMount.call(this); | ||
| public componentWillMount() { | ||
| if (super.componentWillMount != null) { | ||
| super.componentWillMount(); |
There was a problem hiding this comment.
mmm not a huge fan since you'll have to do safeInvoke(super.componentWillMount.bind(this))
There was a problem hiding this comment.
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.
is this expect...equal the same as assert.strictEqual?
There was a problem hiding this comment.
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.
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.
is it possible to support React.ReactNode return types? that would include React.ReactPortal.
There was a problem hiding this comment.
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)
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.
what's the use case for this? if we keep it, it should be called INamed
There was a problem hiding this comment.
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.
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.
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.
should we invoke oldOnContextMenu in this code path?
There was a problem hiding this comment.
Not sure about the logic here; @giladgray ?
There was a problem hiding this comment.
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.
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
HotkeysTargetandContextMenuTargetby returning a new HOC rather than monkeypatching.Reviewers should focus on:
Behavior is maintained from before.