[Refacoring] Make Redux Connect more explicit in Sources Tree#4454
Conversation
wldcordeiro
left a comment
There was a problem hiding this comment.
Some comments but I support moving towards only having the action creators the component needs as well. So this is a good start.
|
|
||
| const mapDispatchToProps = dispatch => { | ||
| return { | ||
| setExpandedState: () => dispatch(setExpandedState()), |
There was a problem hiding this comment.
Redux automatically binds dispatch to an object of functions so we could make this look like
const actionCreators = {
setExpandedState,
selectSource
};
export default connect(mapStateToProps, actionCreators)(SourcesTree);There was a problem hiding this comment.
This is also a great way to do it!
|
|
||
| render() { | ||
| const { setExpandedState, expanded } = this.props; | ||
| const expanded = this.props.expanded; |
There was a problem hiding this comment.
I don't get this change. You could have left it and made onExpand/onCollapse call it without this.props
There was a problem hiding this comment.
The change was necessary after the explicit import of setExpandedState could have also renamed it, I guess.
| highlightItems | ||
| } = this.state; | ||
|
|
||
| const onExpand = (item, expandedState) => { |
There was a problem hiding this comment.
Both the functions are the same so how about toggleExpanded?
There was a problem hiding this comment.
haha - it's small enough where this is fine too. Good either way
|
ping @amelzer |
|
This looks great! cant wait to see it merged! |
|
@jasonLaster @codehag @wldcordeiro So with just the action creators (no usage of dispatch) this would be good to go? I can update it then. |
|
yep |
bfbd3f1 to
bfe099d
Compare
Hey folks, when working on SourcesTree.js I noticed that the redux usage can be made more readable using a more explicit approach.
Summary of Changes
() => dispatch(myImportedAction())I think this makes it more readable and more accessible for newbies who don't have to follow a trail of imports to see where props are coming from.