Add ITreeNode.nodeData property #2132

izikorgad · 2018-02-14T13:52:35Z

Fixes #1989

This feature add a new generics type prop for ITreeNode called 'nodeData'.
The user can use this property to store custom user data, object reference, etc.

Checklist

Enable CircleCI for your fork
Include tests
Update documentation

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

palantirtech · 2018-02-14T13:52:36Z

Thanks for your interest in palantir/blueprint, @izikorgad! 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.

izikorgad · 2018-02-14T13:56:21Z

This PR is an implementation of the concept described as part of #1998 (by @yaronlavi).

palantirtech · 2018-02-14T13:56:27Z

Thanks for your interest in palantir/blueprint, @izikorgad! 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.

giladgray · 2018-02-14T19:04:12Z

@izikorgad please fill out the description up top. that's the place to note the issue number (which i already did for you)

giladgray · 2018-02-14T19:05:03Z

packages/core/src/components/tree/treeNode.tsx

@@ -13,7 +13,7 @@ import { safeInvoke } from "../../common/utils";
 import { Collapse } from "../collapse/collapse";
 import { Icon, IconName } from "../icon/icon";

-export interface ITreeNode extends IProps {
+export interface ITreeNode<T = {}> extends IProps {


can we say T = undefined? more accurate for the default case. if you want to use <T> then you have to define it.

When changing the generic constrain to <T = undefined>, each call to safeInvoke() won't complied:
Argument of type 'this' is not assignable to parameter of type 'TreeNode'.

I don't see the problem with the <T = {}> constrain, the user can still use the tree without declaring on any type.

This was my original intent too, but per @adidahiya requested 16 days ago:
"you can set a default for the generic type parameter so that users who don't need it won't have to specify T.
use nodeData: T where the interface is defined as ITreeNode<T = {}>, similar to queryList:
blueprint/packages/select/src/components/query-list/queryList.tsx (Line 15 in ff76c4a)"

since both of you are reviewers and contributors, I suggest you guys touch bases and sync your request.

i guess {} is fine, since React does the same thing and state doesn't actually default to anything (you gotta initialize it yourself)

giladgray

pushed a commit to fix docs language. looks great 👍

this will ship in the next 2.0 release!

adidahiya · 2018-02-15T20:00:19Z

What does usage of this new feature look like in practice? It would be helpful if you added it to the docs example. I'm pretty sure you still want an ofType utility as mentioned in #1998 (comment)

giladgray · 2018-02-15T20:19:32Z

hmm yeah might want Tree.ofType<T>()

izikorgad · 2018-02-16T11:41:05Z

Sorry, but I’m not following...
Is the ‘ofType()’ is a standard typescript method?
What exactly is the purpose of this function?

Should it be placed inside the Tree class or TreeNode class?

Something like that:

public static ofType<T>() {
    return TreeNode as new () => TreeNode<T>;
}

giladgray · 2018-02-16T18:30:38Z

@izikorgad check out the source code for our select components (and usages thereof, like the examples), that's where we "pioneered" this approach: https://github.com/palantir/blueprint/blob/develop/packages/select/src/components/select/select.tsx#L90-L92

giladgray · 2018-02-16T18:33:21Z

the method is essential for typescript consumers to apply <T> to the generic component.
generic components defy JSX, as they would overload the <> tag symbols, so you have to declare a local variable with the generic parameter applied:

<Select<IFilm> props> // error

const FilmSelect = Select.ofType<IFilm>();
<FilmSelect props> // ok

izikorgad · 2018-02-18T07:43:13Z

OK.
I've added ofType to both Tree and TreeNode components.
Please review.

giladgray · 2018-03-02T00:14:57Z

packages/core/src/components/tree/tree.tsx

@@ -59,6 +63,10 @@ export class Tree extends React.Component<ITreeProps, {}> {

    private nodeRefs: { [nodeId: string]: HTMLElement } = {};

+    constructor(props?: ITreeProps<T>, context?: any) {
+        super(props, context);
+    }


whoa whoa remove this constructor, not necessary!

giladgray · 2018-03-02T00:15:22Z

packages/core/src/components/tree/treeNode.tsx

+        return TreeNode as new () => TreeNode<T>;
+    }
+
+    constructor(props?: ITreeNodeProps<T>, context?: any) {


The code won't compiled without, because of the following function:
public static ofType() {
return TreeNode as new () => TreeNode;
}

It is necessary....

oh snap! yeah it does fail. but that's just a typings issue.

props? in constructor is actually wrong--it goes against React's types and will fail if strictNullChecks is enabled (it is disabled in blueprint).

i'll resolve that in all instances later 👍

Declear nodeData as generics type.

2a5385e

izikorgad closed this Feb 14, 2018

izikorgad reopened this Feb 14, 2018

fix tslint errors.

026d373

izikorgad mentioned this pull request Feb 14, 2018

Add ITreeNode.userData property. #1998

Closed

giladgray reviewed Feb 14, 2018

View reviewed changes

reformat docs

832eaf0

giladgray approved these changes Feb 15, 2018

View reviewed changes

lint

5b9aeb9

Added ofType<T>() to both Tree and TreeNode components.

b5327e7

izikorgad added 6 commits February 19, 2018 09:25

lint

d0fe501

lint

a6b9aa8

lint

6192120

lint

d1544dc

lint

829b0cd

lint

ba66143

adidahiya self-assigned this Feb 19, 2018

giladgray suggested changes Mar 2, 2018

View reviewed changes

giladgray approved these changes Mar 7, 2018

View reviewed changes

giladgray merged commit cc106a0 into palantir:develop Mar 7, 2018

This was referenced Mar 7, 2018

fix ofType syntax #2209

Merged

Prepare Release v2.0.0-rc.3 #2222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ITreeNode.nodeData property #2132

Add ITreeNode.nodeData property #2132

izikorgad commented Feb 14, 2018 •

edited by giladgray

Loading

palantirtech commented Feb 14, 2018

izikorgad commented Feb 14, 2018

palantirtech commented Feb 14, 2018

giladgray commented Feb 14, 2018

giladgray Feb 14, 2018

izikorgad Feb 15, 2018

yaron-lavi-deel Feb 15, 2018

giladgray Feb 15, 2018

giladgray left a comment

adidahiya commented Feb 15, 2018

giladgray commented Feb 15, 2018

izikorgad commented Feb 16, 2018

giladgray commented Feb 16, 2018 •

edited

Loading

giladgray commented Feb 16, 2018

izikorgad commented Feb 18, 2018

giladgray Mar 2, 2018

giladgray Mar 2, 2018

izikorgad Mar 5, 2018

giladgray Mar 7, 2018

Add ITreeNode.nodeData property #2132

Add ITreeNode.nodeData property #2132

Conversation

izikorgad commented Feb 14, 2018 • edited by giladgray Loading

Fixes #1989

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

palantirtech commented Feb 14, 2018

izikorgad commented Feb 14, 2018

palantirtech commented Feb 14, 2018

giladgray commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray left a comment

Choose a reason for hiding this comment

adidahiya commented Feb 15, 2018

giladgray commented Feb 15, 2018

izikorgad commented Feb 16, 2018

giladgray commented Feb 16, 2018 • edited Loading

giladgray commented Feb 16, 2018

izikorgad commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izikorgad commented Feb 14, 2018 •

edited by giladgray

Loading

giladgray commented Feb 16, 2018 •

edited

Loading