-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Add tree view example #1269
Add tree view example #1269
Conversation
This is fantastic. 👍 |
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 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dispatch needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connect()
call on line 60 binds the actions to dispatch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense! Not seen that being used before.
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 Really appreciate you taking the time doing an example for this by the way, thanks! |
A great enhancement to this example would be removing node and moving nodes. |
I'm happy to accept a PR for removing node. Would you like to work on this? |
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). |
There are three possible ways, I see, for removing a node, assuming that you want to remove all of its children from the store...
I did |
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. |
Maybe my data structure is what's confusing me here. I looked back at the example and noticed the multiple 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: 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 😉 |
Node Removal PR #1274. |
Ah yes thanks @erikras I will look at the PR for reference. Merci 😄 |
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. |
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 |
Yes, I think this can work fine. |
👍 Merci beaucoup mon ami ! |
} | ||
} | ||
|
||
export function addChild(nodeId, childId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try and submit a PR if it works. It's hard to discuss hypothetical code. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I'll do it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem 😉 If you do find a way to make this code simpler let us know!
This adds a performant tree view example by a popular request.
Answers https://stackoverflow.com/questions/34981924/performance-issues-with-a-tree-structure-and-shouldcomponentupdate-in-react-re.