Add tree view example #1269

Merged
merged 1 commit into from Jan 25, 2016

Conversation

10 participants
@gaearon
Collaborator

gaearon commented Jan 25, 2016

@gaearon gaearon added the examples label Jan 25, 2016

gaearon added a commit that referenced this pull request Jan 25, 2016

@gaearon gaearon merged commit e3bae90 into master Jan 25, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gaearon gaearon deleted the tree-view-example branch Jan 25, 2016

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Jan 25, 2016

Contributor

This is fantastic. 👍

Contributor

erikras commented Jan 25, 2016

This is fantastic. 👍

@CrocoDillon

This comment has been minimized.

Show comment
Hide comment
@CrocoDillon

CrocoDillon Jan 25, 2016

Contributor

Agreed, this example covers so much 😄

I like how you break “convention” by putting action types in the action file instead of a constants file which makes sense. Also your root reducer doesn’t care about action types since it does duck typing instead (check for nodeId on the action).

Also many people think that you should have few connected components, for example only root. And here you have 1000 connected components! 😁

I think examples like this by influential people like you are good, shows the community boilerplate can be reduced by not sticking to convention when it doesn’t make sense for your use-case.

Contributor

CrocoDillon commented Jan 25, 2016

Agreed, this example covers so much 😄

I like how you break “convention” by putting action types in the action file instead of a constants file which makes sense. Also your root reducer doesn’t care about action types since it does duck typing instead (check for nodeId on the action).

Also many people think that you should have few connected components, for example only root. And here you have 1000 connected components! 😁

I think examples like this by influential people like you are good, shows the community boilerplate can be reduced by not sticking to convention when it doesn’t make sense for your use-case.

+
+ handleIncrementClick() {
+ const { increment, id } = this.props
+ increment(id)

This comment has been minimized.

@stephenwf

stephenwf Jan 25, 2016

No dispatch needed?

@stephenwf

stephenwf Jan 25, 2016

No dispatch needed?

This comment has been minimized.

@erikras

erikras Jan 25, 2016

Contributor

The connect() call on line 60 binds the actions to dispatch.

@erikras

erikras Jan 25, 2016

Contributor

The connect() call on line 60 binds the actions to dispatch.

This comment has been minimized.

@stephenwf

stephenwf Jan 25, 2016

Ah, that makes sense! Not seen that being used before.

@stephenwf

stephenwf Jan 25, 2016

Ah, that makes sense! Not seen that being used before.

+* Normalized state
+* Reducer composition
+* State representing a tree view
+* Granual re-rendering of a large subtree

This comment has been minimized.

@glenjamin

glenjamin Jan 25, 2016

nit: "Granular" :) (or Gradual, i suppose)

@glenjamin

glenjamin Jan 25, 2016

nit: "Granular" :) (or Gradual, i suppose)

This comment has been minimized.

@erikras

erikras Jan 25, 2016

Contributor

Probably not "annual". If you want UI that slow, use Angular! 😆

@erikras

erikras Jan 25, 2016

Contributor

Probably not "annual". If you want UI that slow, use Angular! 😆

This comment has been minimized.

@gaearon

gaearon Jan 25, 2016

Collaborator

Fix is welcome. That's just 5am me

@gaearon

gaearon Jan 25, 2016

Collaborator

Fix is welcome. That's just 5am me

+ increment(id)
+ }
+
+ handleAddChildClick(e) {

This comment has been minimized.

@sompylasar

sompylasar Jan 25, 2016

Shouldn't this be handleAddChildClick = (e) => { to bind this properly, or am I missing something? The same for handleIncrementClick.

@sompylasar

sompylasar Jan 25, 2016

Shouldn't this be handleAddChildClick = (e) => { to bind this properly, or am I missing something? The same for handleIncrementClick.

This comment has been minimized.

@gaearon

gaearon Jan 25, 2016

Collaborator

We bind these functions in the constructor. It's more efficient (which is one of the points of this example) than allocating a function on every render.

@gaearon

gaearon Jan 25, 2016

Collaborator

We bind these functions in the constructor. It's more efficient (which is one of the points of this example) than allocating a function on every render.

This comment has been minimized.

@sompylasar

sompylasar Jan 25, 2016

Ah, missed that, sorry. Property initializers should do the same, but I noticed you prefer not to use them.

@sompylasar

sompylasar Jan 25, 2016

Ah, missed that, sorry. Property initializers should do the same, but I noticed you prefer not to use them.

This comment has been minimized.

@gaearon

gaearon Jan 25, 2016

Collaborator

We settled on only using ES6 features in examples a while ago. Mostly to stop alienating newcomers who are confused by the ecosystem already and assume Redux requires knowledge of ES2016+ proposals.

@gaearon

gaearon Jan 25, 2016

Collaborator

We settled on only using ES6 features in examples a while ago. Mostly to stop alienating newcomers who are confused by the ecosystem already and assume Redux requires knowledge of ES2016+ proposals.

This comment has been minimized.

@sompylasar

sompylasar Jan 25, 2016

Yes, I remember that. Thanks for clarification!

@sompylasar

sompylasar Jan 25, 2016

Yes, I remember that. Thanks for clarification!

@guilambert

This comment has been minimized.

Show comment
Hide comment
@guilambert

guilambert Jan 25, 2016

Nice example, I'm wondering tho how an action affecting the entire state would be handled? For example removing a node, which in turn affects the entire tree?

I need to get my head around this for optimization purpose, I did encounter performance issues with a somewhat big tree of data.

Say for example I have a <Selected /> component that stores added items from a list, when added it shows up in the Selection component, but in the list shows as "added", and vice-versa. How would that need to be handled in the tree-view example?

Really appreciate you taking the time doing an example for this by the way, thanks!

Nice example, I'm wondering tho how an action affecting the entire state would be handled? For example removing a node, which in turn affects the entire tree?

I need to get my head around this for optimization purpose, I did encounter performance issues with a somewhat big tree of data.

Say for example I have a <Selected /> component that stores added items from a list, when added it shows up in the Selection component, but in the list shows as "added", and vice-versa. How would that need to be handled in the tree-view example?

Really appreciate you taking the time doing an example for this by the way, thanks!

@tamagokun

This comment has been minimized.

Show comment
Hide comment
@tamagokun

tamagokun Jan 25, 2016

A great enhancement to this example would be removing node and moving nodes.

A great enhancement to this example would be removing node and moving nodes.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 25, 2016

Collaborator

I'm happy to accept a PR for removing node. Would you like to work on this?

Collaborator

gaearon commented Jan 25, 2016

I'm happy to accept a PR for removing node. Would you like to work on this?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 25, 2016

Collaborator

Nice example, I'm wondering tho how an action affecting the entire state would be handled? For example removing a node, which in turn affects the entire tree?

Why does removing affect the entire tree? Seems like it would only affect the removed node's parent (and, of course, its children, but they would just get unmounted).

Collaborator

gaearon commented Jan 25, 2016

Nice example, I'm wondering tho how an action affecting the entire state would be handled? For example removing a node, which in turn affects the entire tree?

Why does removing affect the entire tree? Seems like it would only affect the removed node's parent (and, of course, its children, but they would just get unmounted).

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Jan 25, 2016

Contributor

There are three possible ways, I see, for removing a node, assuming that you want to remove all of its children from the store...

  1. Actually keep the data in a tree structure in the store, so removing a node object automatically removes all its children, which it contained.
  2. Maintain the flat structure and give them string ids like '1.5.23.1' so that the reducer can just remove all ids that start with the removed node's id.
  3. Maintain the flat structure and depth-first iterate on the removal through all the child nodes.

I did #1 in my redux-form library, which added much complication. I think I'd recommend #3 for this example.

Contributor

erikras commented Jan 25, 2016

There are three possible ways, I see, for removing a node, assuming that you want to remove all of its children from the store...

  1. Actually keep the data in a tree structure in the store, so removing a node object automatically removes all its children, which it contained.
  2. Maintain the flat structure and give them string ids like '1.5.23.1' so that the reducer can just remove all ids that start with the removed node's id.
  3. Maintain the flat structure and depth-first iterate on the removal through all the child nodes.

I did #1 in my redux-form library, which added much complication. I think I'd recommend #3 for this example.

@bramdevries

This comment has been minimized.

Show comment
Hide comment
@bramdevries

bramdevries Jan 25, 2016

Here's a method I used to remove nodes on my own project (uses Immutable.js) but using the same flat structure:

const parentIds = state.get('tree').reduce((result, item) => {
    if (item.get('children').includes(action.id)) {
        result.push(item.get('id'));
    }
    return result;
}, []);

parentIds.forEach(id => {
    const path = ['tree', String(id), 'children'];
    path.push(state.getIn(path).findIndex(i => i === action.id));
    state = state.removeIn(path);
});

return state.removeIn(['tree', String(action.id)]);

Basically what happens is that we run through all nodes and get the id's of where the removed node is a child, then we loop over all these nodes and remove the reference. Finally we remove the actual node.

This is not 100% perfect though, for example. if you remove a node that still has children it won't remove those nodes.

Here's a method I used to remove nodes on my own project (uses Immutable.js) but using the same flat structure:

const parentIds = state.get('tree').reduce((result, item) => {
    if (item.get('children').includes(action.id)) {
        result.push(item.get('id'));
    }
    return result;
}, []);

parentIds.forEach(id => {
    const path = ['tree', String(id), 'children'];
    path.push(state.getIn(path).findIndex(i => i === action.id));
    state = state.removeIn(path);
});

return state.removeIn(['tree', String(action.id)]);

Basically what happens is that we run through all nodes and get the id's of where the removed node is a child, then we loop over all these nodes and remove the reference. Finally we remove the actual node.

This is not 100% perfect though, for example. if you remove a node that still has children it won't remove those nodes.

@guilambert

This comment has been minimized.

Show comment
Hide comment
@guilambert

guilambert Jan 25, 2016

Maybe my data structure is what's confusing me here. I looked back at the example and noticed the multiple connect()ed components instead of only one HOC. I always thoughts this was best practice, so basically what I've been doing is connect the entire state to the top-most container, and then pass along state and dispatch to the children as props:

class FilesDownload extends Component {

  render() {
    return (
      <div className="files-download">
        <div className="downloads">
          <div className="downloads__selection">
            <Selection {...this.props} />
          </div>
          <div className="downloads__list">
            <Filters {...this.props} />
            <Results {...this.props} />
          </div>
        </div>
      </div>
    );
  }

}

function select(state) {
  return state;
}

// Wrap the component to inject dispatch and state into it
export default connect(select)(FilesDownload);

Here's an example Gif of the actual app, you can see the delay when I first click on the "add to selection" button:

redux

EDIT: as you can see too, once the first action happens, then things run smoothly. I know I'm missing something here but can't get my head around it. I need to keep digging 😉

Maybe my data structure is what's confusing me here. I looked back at the example and noticed the multiple connect()ed components instead of only one HOC. I always thoughts this was best practice, so basically what I've been doing is connect the entire state to the top-most container, and then pass along state and dispatch to the children as props:

class FilesDownload extends Component {

  render() {
    return (
      <div className="files-download">
        <div className="downloads">
          <div className="downloads__selection">
            <Selection {...this.props} />
          </div>
          <div className="downloads__list">
            <Filters {...this.props} />
            <Results {...this.props} />
          </div>
        </div>
      </div>
    );
  }

}

function select(state) {
  return state;
}

// Wrap the component to inject dispatch and state into it
export default connect(select)(FilesDownload);

Here's an example Gif of the actual app, you can see the delay when I first click on the "add to selection" button:

redux

EDIT: as you can see too, once the first action happens, then things run smoothly. I know I'm missing something here but can't get my head around it. I need to keep digging 😉

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Jan 25, 2016

Contributor

Node Removal PR #1274.

Contributor

erikras commented Jan 25, 2016

Node Removal PR #1274.

@guilambert

This comment has been minimized.

Show comment
Hide comment
@guilambert

guilambert Jan 25, 2016

Ah yes thanks @erikras I will look at the PR for reference. Merci 😄

Ah yes thanks @erikras I will look at the PR for reference. Merci 😄

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 27, 2016

Collaborator

I always thoughts this was best practice, so basically what I've been doing is connect the entire state to the top-most container, and then pass along state and dispatch to the children as props:

Yeah, we no longer recommend single top container in favor of a more gradual approach. See the updated https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.49376my94.

Collaborator

gaearon commented Jan 27, 2016

I always thoughts this was best practice, so basically what I've been doing is connect the entire state to the top-most container, and then pass along state and dispatch to the children as props:

Yeah, we no longer recommend single top container in favor of a more gradual approach. See the updated https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.49376my94.

@guilambert

This comment has been minimized.

Show comment
Hide comment
@guilambert

guilambert Jan 27, 2016

Yes thanks for the reminder, as I tweeted earlier (tweet which you "liked") I'm slowing getting the hang of this. It's really starting to make sense now with Presentational and Container Components.

Perhaps, with my example above, I should have a connect() container for each selectable items (CAD Drawing, Install Drawings etc.), what's your thoughts on this?

Yes thanks for the reminder, as I tweeted earlier (tweet which you "liked") I'm slowing getting the hang of this. It's really starting to make sense now with Presentational and Container Components.

Perhaps, with my example above, I should have a connect() container for each selectable items (CAD Drawing, Install Drawings etc.), what's your thoughts on this?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 27, 2016

Collaborator

Perhaps, with my example above, I should have a connect() container for each selectable items (CAD Drawing, Install Drawings etc.), what's your thoughts on this?

Yes, I think this can work fine.

Collaborator

gaearon commented Jan 27, 2016

Perhaps, with my example above, I should have a connect() container for each selectable items (CAD Drawing, Install Drawings etc.), what's your thoughts on this?

Yes, I think this can work fine.

@guilambert

This comment has been minimized.

Show comment
Hide comment
@guilambert

guilambert Jan 27, 2016

👍 Merci beaucoup mon ami !

👍 Merci beaucoup mon ami !

+ }
+}
+
+export function addChild(nodeId, childId) {

This comment has been minimized.

@fhelwanger

fhelwanger Jan 27, 2016

Just wondering... couldn't this action creator receive just nodeId and generate childId internally?

This way, the component doesn't need to call createNode just to get the new id.

@fhelwanger

fhelwanger Jan 27, 2016

Just wondering... couldn't this action creator receive just nodeId and generate childId internally?

This way, the component doesn't need to call createNode just to get the new id.

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Collaborator

This is what I tried at first but it made the reducer a little bit awkward. It's hard to say which approach is cleaner until the app grows large enough and more use cases are implemented so I went with this version that keeps the reducers simpler.

@gaearon

gaearon Jan 27, 2016

Collaborator

This is what I tried at first but it made the reducer a little bit awkward. It's hard to say which approach is cleaner until the app grows large enough and more use cases are implemented so I went with this version that keeps the reducers simpler.

This comment has been minimized.

@fhelwanger

fhelwanger Jan 27, 2016

I don't get it, I was thinking about something like this:

export function createNode() {
  return {
    type: CREATE_NODE,
    nodeId: generateId()
  }
}

export function addChild(nodeId) {
  return {
    type: ADD_CHILD,
    nodeId,
    childId: generateId()
  }
}

let nextId = 0
function generateId() {
  return `new_${nextId++}`
}

How does it make the reducers more complex?

Sorry if I'm missing something 😄

@fhelwanger

fhelwanger Jan 27, 2016

I don't get it, I was thinking about something like this:

export function createNode() {
  return {
    type: CREATE_NODE,
    nodeId: generateId()
  }
}

export function addChild(nodeId) {
  return {
    type: ADD_CHILD,
    nodeId,
    childId: generateId()
  }
}

let nextId = 0
function generateId() {
  return `new_${nextId++}`
}

How does it make the reducers more complex?

Sorry if I'm missing something 😄

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Collaborator

Please try and submit a PR if it works. It's hard to discuss hypothetical code. ;-)

@gaearon

gaearon Jan 27, 2016

Collaborator

Please try and submit a PR if it works. It's hard to discuss hypothetical code. ;-)

This comment has been minimized.

@fhelwanger

fhelwanger Jan 27, 2016

Nice, I'll do it :)

@fhelwanger

fhelwanger Jan 27, 2016

Nice, I'll do it :)

This comment has been minimized.

@fhelwanger

fhelwanger Jan 27, 2016

Oh, I got it now 😓

When you call const childId = createNode().nodeId you're actually dispatching the action because of mapDispatchToProps.

When reading the code, I assumed it was being called only to get the new id.

You were right, it's hard to discuss hypothetical code :-)

Sorry for asking without testing first, and thank you so much for all the good work you've been doing 😄

@fhelwanger

fhelwanger Jan 27, 2016

Oh, I got it now 😓

When you call const childId = createNode().nodeId you're actually dispatching the action because of mapDispatchToProps.

When reading the code, I assumed it was being called only to get the new id.

You were right, it's hard to discuss hypothetical code :-)

Sorry for asking without testing first, and thank you so much for all the good work you've been doing 😄

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Collaborator

No problem 😉 If you do find a way to make this code simpler let us know!

@gaearon

gaearon Jan 27, 2016

Collaborator

No problem 😉 If you do find a way to make this code simpler let us know!

@markerikson markerikson referenced this pull request May 31, 2016

Closed

FAQ updates #1785

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment