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.

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

Added tree-view node removal, as requested on #1269 #1274

Closed
wants to merge 2 commits into from
Closed

Added tree-view node removal, as requested on #1269 #1274

wants to merge 2 commits into from

Conversation

erikras
Copy link
Contributor

@erikras erikras commented Jan 25, 2016

Also put the id in the title prop for node identification.

@erikras erikras mentioned this pull request Jan 25, 2016
@CrocoDillon
Copy link
Contributor

I was also giving this a shot. However it looks like you're not really removing the (child)nodes, just remove the node id from it parent's childIds list... right?

@erikras
Copy link
Contributor Author

erikras commented Jan 25, 2016

Oh! Yes, you're right.

@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

I think it would be easier to pass { childId, nodeId } to the action where nodeId is the ID of the parent. The Node component can pass parentId to its child Node components as a prop, and children will use that when the fire the action. Then we don't need to search for the parent.

@CrocoDillon
Copy link
Contributor

My take on this in #1275, however I don't like the heavy binding... plus I'm also not removing the node. Anyway bedtime for me so maybe I'll continue tomorrow. Unless you finish it first which is more likely 😉

@erikras
Copy link
Contributor Author

erikras commented Jan 25, 2016

@gaearon Does the Node component have its parent's id, though? I guess it could be made to.

@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

#1275 is more like what I had in mind. Thanks!

@gaearon gaearon closed this Jan 25, 2016
@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

I guess it could be made to.

Yeah, if we make Node pass it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants