Skip to content
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.

Sign up for GitHub

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

Fix support for generic types in Tree component #2629

Merged
merged 8 commits into from
Jul 11, 2018
Merged

Fix support for generic types in Tree component #2629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
20d8328
TreeEventHandler inherits generic types
Jun 25, 2018
d8a903e
add generics to TreeNode
Jun 25, 2018
b9d84ba
add nonstatic nodeFromPath for generics and fix typings
Jun 27, 2018
10eec63
style fixes
Jun 27, 2018
54a91d0
indentation
Jun 27, 2018
1112d7f
remove underscore on iterator variable
Jun 27, 2018
30a9638
move render up
Jun 27, 2018
a4b1b5a
remove nonstatic nodefrompath
Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions packages/core/src/components/tree/tree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import { IProps } from "../../common/props";
import { isFunction } from "../../common/utils";
import { ITreeNode, TreeNode } from "./treeNode";

export type TreeEventHandler = (node: ITreeNode, nodePath: number[], e: React.MouseEvent<HTMLElement>) => void;
export type TreeEventHandler<T = {}> = (
node: ITreeNode<T>,
nodePath: number[],
e: React.MouseEvent<HTMLElement>,
) => void;

export interface ITreeProps<T = {}> extends IProps {
/**
Expand All @@ -23,29 +27,29 @@ export interface ITreeProps<T = {}> extends IProps {
/**
* Invoked when a node is clicked anywhere other than the caret for expanding/collapsing the node.
*/
onNodeClick?: TreeEventHandler;
onNodeClick?: TreeEventHandler<T>;

/**
* Invoked when caret of an expanded node is clicked.
*/
onNodeCollapse?: TreeEventHandler;
onNodeCollapse?: TreeEventHandler<T>;

/**
* Invoked when a node is right-clicked or the context menu button is pressed on a focused node.
*/
onNodeContextMenu?: TreeEventHandler;
onNodeContextMenu?: TreeEventHandler<T>;

/**
* Invoked when a node is double-clicked. Be careful when using this in combination with
* an `onNodeClick` (single-click) handler, as the way this behaves can vary between browsers.
* See http://stackoverflow.com/q/5497073/3124288
*/
onNodeDoubleClick?: TreeEventHandler;
onNodeDoubleClick?: TreeEventHandler<T>;

/**
* Invoked when the caret of a collapsed node is clicked.
*/
onNodeExpand?: TreeEventHandler;
onNodeExpand?: TreeEventHandler<T>;
}

export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> {
Expand Down Expand Up @@ -80,15 +84,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't have to do this. can you revert this safely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (
<TreeNode
<TypedTreeNode
{...node}
key={node.id}
contentRef={this.handleContentRef}
Expand All @@ -101,22 +106,22 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> {
path={elementPath}
>
{this.renderNodes(node.childNodes, elementPath)}
</TreeNode>
</TypedTreeNode>
);
});

return <ul className={classNames(Classes.TREE_NODE_LIST, className)}>{nodeItems}</ul>;
}

private handleNodeCollapse = (node: TreeNode, e: React.MouseEvent<HTMLElement>) => {
private handleNodeCollapse = (node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) => {
this.handlerHelper(this.props.onNodeCollapse, node, e);
};

private handleNodeClick = (node: TreeNode, e: React.MouseEvent<HTMLElement>) => {
private handleNodeClick = (node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) => {
this.handlerHelper(this.props.onNodeClick, node, e);
};

private handleContentRef = (node: TreeNode, element: HTMLElement | null) => {
private handleContentRef = (node: TreeNode<T>, element: HTMLElement | null) => {
if (element != null) {
this.nodeRefs[node.props.id] = element;
} else {
Expand All @@ -125,19 +130,19 @@ export class Tree<T = {}> extends React.Component<ITreeProps<T>, {}> {
}
};

private handleNodeContextMenu = (node: TreeNode, e: React.MouseEvent<HTMLElement>) => {
private handleNodeContextMenu = (node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) => {
this.handlerHelper(this.props.onNodeContextMenu, node, e);
};

private handleNodeDoubleClick = (node: TreeNode, e: React.MouseEvent<HTMLElement>) => {
private handleNodeDoubleClick = (node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) => {
this.handlerHelper(this.props.onNodeDoubleClick, node, e);
};

private handleNodeExpand = (node: TreeNode, e: React.MouseEvent<HTMLElement>) => {
private handleNodeExpand = (node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) => {
this.handlerHelper(this.props.onNodeExpand, node, e);
};

private handlerHelper(handlerFromProps: TreeEventHandler, node: TreeNode, e: React.MouseEvent<HTMLElement>) {
private handlerHelper(handlerFromProps: TreeEventHandler, node: TreeNode<T>, e: React.MouseEvent<HTMLElement>) {
if (isFunction(handlerFromProps)) {
const nodeData = Tree.nodeFromPath(node.props.path, this.props.contents);
handlerFromProps(nodeData, node.props.path, e);
Expand Down
16 changes: 7 additions & 9 deletions packages/core/src/components/tree/treeNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface ITreeNode<T = {}> extends IProps {
/**
* Child tree nodes of this node.
*/
childNodes?: ITreeNode[];
childNodes?: Array<ITreeNode<T>>;

/**
* Whether the caret to expand/collapse a node should be shown.
Expand All @@ -36,8 +36,6 @@ export interface ITreeNode<T = {}> extends IProps {
id: string | number;

/**
* Whether the children of this node are displayed.
* @default false
*/
isExpanded?: boolean;

Expand Down Expand Up @@ -67,14 +65,14 @@ export interface ITreeNode<T = {}> extends IProps {

export interface ITreeNodeProps<T = {}> extends ITreeNode<T> {
children?: React.ReactNode;
contentRef?: (node: TreeNode, element: HTMLDivElement | null) => void;
contentRef?: (node: TreeNode<T>, element: HTMLDivElement | null) => void;
depth: number;
key?: string | number;
onClick?: (node: TreeNode, e: React.MouseEvent<HTMLDivElement>) => void;
onCollapse?: (node: TreeNode, e: React.MouseEvent<HTMLSpanElement>) => void;
onContextMenu?: (node: TreeNode, e: React.MouseEvent<HTMLDivElement>) => void;
onDoubleClick?: (node: TreeNode, e: React.MouseEvent<HTMLDivElement>) => void;
onExpand?: (node: TreeNode, e: React.MouseEvent<HTMLSpanElement>) => void;
onClick?: (node: TreeNode<T>, e: React.MouseEvent<HTMLDivElement>) => void;
onCollapse?: (node: TreeNode<T>, e: React.MouseEvent<HTMLSpanElement>) => void;
onContextMenu?: (node: TreeNode<T>, e: React.MouseEvent<HTMLDivElement>) => void;
onDoubleClick?: (node: TreeNode<T>, e: React.MouseEvent<HTMLDivElement>) => void;
onExpand?: (node: TreeNode<T>, e: React.MouseEvent<HTMLSpanElement>) => void;
path: number[];
}

Expand Down