Reimplemented Callhierarchy Tree Widget with use of React #2224
Conversation
Signed-off-by: Jan Bicker <jan.bicker@typefox.io>
jbicker
changed the title
| @@ -0,0 +1,715 @@ | |||
| /******************************************************************************** | |||
| * Copyright (C) 2017 TypeFox and others. | |||
| export const TREE_NODE_CAPTION_CLASS = 'theia-TreeNodeCaption'; | ||
| export const EXPANSION_TOGGLE_CLASS = 'theia-ExpansionToggle'; | ||
|
|
||
| export const TreeProps = Symbol('TreeProps'); |
There was a problem hiding this comment.
You either have to have the TreeReactProps or remove this.
| } | ||
| } | ||
| onClick={this.toggle}> | ||
| </div >; |
|
|
||
| protected readonly toggle = (event: React.MouseEvent<HTMLElement>) => this.doToggle(event); | ||
| protected doToggle(event: React.MouseEvent<HTMLElement>) { | ||
| const nodeId = event.currentTarget.getAttribute('data-node-id'); |
There was a problem hiding this comment.
data-node-id: please use constants.
There was a problem hiding this comment.
In this case this attribute should only be used here! It should not be reused or renamed. In the JSX below it has to be written as it is. So why use a constant for it?
| protected readonly toggle = (event: React.MouseEvent<HTMLElement>) => this.doToggle(event); | ||
| protected doToggle(event: React.MouseEvent<HTMLElement>) { | ||
| const nodeId = event.currentTarget.getAttribute('data-node-id'); | ||
| const node = this.model.getNode(nodeId || undefined); |
There was a problem hiding this comment.
This nodeId || undefined looks odd. Please add a guard instead.
if (nodeId) {
const node = this.mode.getNode(nodeId);
this.handleClickEvent(node, event.nativeEvent);
event.stopPropagation();
}| event.stopPropagation(); | ||
| } | ||
|
|
||
| protected handleContextMenuEvent(node: TreeNode | undefined, event: React.MouseEvent<HTMLElement>): void { |
There was a problem hiding this comment.
Why do you use React.MouseEvent here and the native one (MouseEvent) for other methods. For instance; handleDblClickEvent. If there is no reason, please try to ensure a symmetric API.
| }; | ||
| } | ||
|
|
||
| protected onAfterAttach(msg: Message): void { |
There was a problem hiding this comment.
[minor]: Can you please move this down under the getDecorationData<K extends keyof TreeDecoration.Data>(node: TreeNode, key: K): TreeDecoration.Data[K][] method? It would be easier to see and compare the differences.
| this.addKeyListener(this.node, Key.ARROW_RIGHT, event => this.handleRight(event)); | ||
| this.addKeyListener(this.node, up, event => this.handleUp(event)); | ||
| this.addKeyListener(this.node, down, event => this.handleDown(event)); | ||
| this.addKeyListener(this.node, Key.ENTER, event => this.handleEnter(event)); |
There was a problem hiding this comment.
There was/is a this.addEventListener(this.node, 'contextmenu', e => this.handleContextMenuEvent(this.model.root, e)); line in the TreeWidget. What has happened to that?
There was a problem hiding this comment.
We had trouble with mixing up native and react events. So the event is now registered here.
There was a problem hiding this comment.
Thanks for the link and the explanation. Can you please give me some insight into this trouble?
There was a problem hiding this comment.
React delegates every event which is registered on a react element to the document root. Here are the handlers stored and called. So if we register a handler on some tree node via react and register a handler for the same event on the root node not via react (like in that old case) the handler of the tree root node is processed. And because of the stopPropagation the event does not reach the document root and the handlers of the tree nodes aren't called.
That is how I understood this situation.
More about that: facebook/react#7094 (comment)
...and: https://davidwalsh.name/event-delegate
There was a problem hiding this comment.
I need to try this out. Thanks!
| const attrs = super.createContainerAttributes(); | ||
| return { | ||
| ...attrs, | ||
| // onDragEnter: event => this.handleDragEnterEvent(this.model.root, event.nativeEvent), |
There was a problem hiding this comment.
Why is it commented out?
There was a problem hiding this comment.
Forgot to delete it completely.
| @@ -37,6 +37,7 @@ export abstract class ReactWidget extends BaseWidget { | |||
|
|
|||
| protected onUpdateRequest(msg: Message): void { | |||
| super.onUpdateRequest(msg); | |||
|
|
|||
Signed-off-by: Jan Bicker <jan.bicker@typefox.io>
jbicker
force-pushed
the
callhierarchy-react-tree
branch
from
f7aff9b
to
70d8e8f
Compare
This is a branch of #2205. Please review that first.