Remove node for tree-view example

[CrocoDillon]

Requested in #1269 for comparison against this alternative #1274 😄

[CrocoDillon]

Beside the uglyness I probably should have named this prop handleRemoveClick.

[gaearon]

Binding is going to be expensive here because it's called a lot of times.
The child already knows its ID so there is no need to bind here—it can supply it as an argument by reading the props.

[erikras]

Neither are you, though. :-)

The proper algorithm needs to create a list of the node to be removed and all of its descendants, and then remove all of those from the node array as well as remove the top node from its parent.

[gaearon]

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

[gaearon]

Argh, I meant to close #1274 :D

[CrocoDillon]

@erikras yeah I just uploaded what I have so far to compare to yours :D

@gaearon I won't have time to finish this today, feel free to go with Erik's PR instead if you both can't wait!

[erikras]

Seems like you still need to slaughter all the grandkids. I don't see how you can do this in only the node() reducer. Just snipping the branch isn't good enough; you have to iterate though all the descendants and remove them as well.

This is why we use data structure libraries. :-)

[gaearon]

I'll leave this to you to finish, no rush.

[gaearon]

👍 I think it's going in the right direction. Might want to extract something like getAllDescendantIds(state, nodeId) and deleteMany(state, ids) when you implement recursive removal.

[CrocoDillon]

Sure thing, I just got home and was about to finish this 😄 And I did notice a semicolon slipped in, power of habit... not a big fan of semicolon-less myself yet.

[CrocoDillon]

The getAllDescendantIds function could probably be optimized since it’s creating a new array for every childId. But either way is this a little bit like what you had in mind?

[CrocoDillon]

@gaearon got time to take another look at this?

[gaearon]

For the sake of consistency please use a typeof check here instead.

[gaearon]

Apart from a few bits this looks great to me. A nice extra would be to add some tests for the reducers and the helper functions but no pressures from you don't have time :-)

[CrocoDillon]

Will do as soon as I find some time 😬

By the way is there a specific reason for using Object.assign instead of spread?

Also would you do something like const node = combineReducers({ id, counter, childIds }) or keep it simple and extract only a childIds reducer?

[gaearon]

By the way is there a specific reason for using Object.assign instead of spread?

We only use ES6 features in official examples. Array spread is ES6 but object spread is only a proposal now.

[gaearon]

As for childIds I didn't mean it to be top-level reducer. I meant to extract a function from node that operates just on that field. Reducer composition. This reducer wouldn't be used anywhere else.

[CrocoDillon]

This reducer wouldn't be used anywhere else.

Also not what I meant but you answered my question anyway :)

[CrocoDillon]

@gaearon I added some basic tests for the root reducer. Those should also cover the reducer’s helper functions.

I didn’t test edge or non happy flow cases, don’t know how far you want to go for examples.

[gaearon]

This looks great!

[CrocoDillon]

Thanks, also made the linter happy now :)

[CrocoDillon]

Need anything else @gaearon? Maybe a DECREMEMT action while at it? ;)

[gaearon]

Mind squashing into a single commit?

[CrocoDillon]

Done!

[gaearon]

A few things in master changed.
Please would you:

1. Rebase on master
2. Add babel-register as dep and use it instead of babel/register
3. Use crossenv when specifying NODE_ENV
4. Tweak Delete to only be visible when Node is hovered? Otherwise there's too much noise.
5. Squash

Thanks again!

[CrocoDillon]

You’re killing me! 😧

Just kidding, will do tonight.

[gaearon]

Just did that myself (toned the color down instead of hover, works OK).

[CrocoDillon]

I was honestly kidding but thanks man!

[gaearon]

No problem. I wanted to get it in ASAP before we blocked it with something else :-)
Thank you for your contribution!