Fix support for generic types in Tree component #2629
Conversation
|
Thanks for your interest in palantir/blueprint, @jjhong922! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
| @@ -80,15 +92,16 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> { | |||
| return this.nodeRefs[nodeId]; | |||
| } | |||
|
|
|||
| private renderNodes(treeNodes: ITreeNode[], currentPath?: number[], className?: string): JSX.Element { | |||
| private renderNodes(treeNodes: Array<ITreeNode<T>>, currentPath?: number[], className?: string): JSX.Element { | |||
There was a problem hiding this comment.
is this change necessary? i don't think it is as this component doesn't care about the <T> type: it's just for the consumer. let's try to keep this as minimal as possible.
There was a problem hiding this comment.
Since the method is called from the render method which passes in this.props.contents, the renderNodes function has to take in typed interfaces to render typed corresponding TypedTreeNodes. The problem that existed was that a TypedTree rendered nodes that would not be able to take in nodeData of the passed in type as expected.
There was a problem hiding this comment.
🤔 the tree nodes are rendered internally by the tree so we don't actually need the type information inside the component (it's only useful in event handlers).
There was a problem hiding this comment.
But since renderNodes is the function that passes down the onClick, onContextMenu, etc. handlers, it cannot create default TreeNodes unless the event handlers match up.
| if (treeNodes == null) { | ||
| return null; | ||
| } | ||
|
|
||
| const nodeItems = treeNodes.map((node, i) => { | ||
| const elementPath = currentPath.concat(i); | ||
| const TypedTreeNode = TreeNode.ofType<T>(); |
There was a problem hiding this comment.
shouldn't have to do this. can you revert this safely?
There was a problem hiding this comment.
This change follows the renderNodes argument type change. I believe some sort of Typed Tree Node is necessary, but I'm not sure if there is a cleaner way.
| @@ -63,6 +67,14 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> { | |||
|
|
|||
| private nodeRefs: { [nodeId: string]: HTMLElement } = {}; | |||
|
|
|||
| public nodeFromPath(path: number[]): ITreeNode<T> { | |||
| let treeNodes = this.props.contents; | |||
| for (let _i = 0; _i < path.length - 1; _i++) { | |||
There was a problem hiding this comment.
call it i cuz it's used. leading _ is convention for unused but necessary variables.
| @@ -63,6 +67,14 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> { | |||
|
|
|||
| private nodeRefs: { [nodeId: string]: HTMLElement } = {}; | |||
|
|
|||
| public nodeFromPath(path: number[]): ITreeNode<T> { | |||
There was a problem hiding this comment.
i'm confused, where was this function defined before?
There was a problem hiding this comment.
also please put this after render(). render should always be the first public method in a react component.
There was a problem hiding this comment.
This function was not defined before, I just made it as a typed alternative to the static nodeFromPath. I can remove it if you don't think it's necessary though.
There was a problem hiding this comment.
oh yes please remove. the instance method is far less preferable than the static one because it requires grabbing a ref.
| @@ -71,6 +75,14 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> { | |||
| ); | |||
| } | |||
|
|
|||
| public nodeFromPath(path: number[]): ITreeNode<T> { | |||
There was a problem hiding this comment.
but actually remove this. we already have the static method.
There was a problem hiding this comment.
oh sounds good, didn't see the hidden comment :P
|
@giladgray Is the test that failed a flaky one? Is it possible you could rerun CI? |
|
thanks @jjhong922! |
Changes proposed in this pull request:
addresses #2627