Update todomvc example #1480
Update todomvc example #1480
Conversation
|
|
||
| const VisibleTodoList = connect( | ||
| mapStateToProps, | ||
| mapDispatchToProps |
There was a problem hiding this comment.
I think we can just pass action creators as object here for brevity. It would be equivalent to mapDispatchToProps in this case.
There was a problem hiding this comment.
agreed, I'll make this change.
There was a problem hiding this comment.
Dan, just to clarify, would the way to do this be using bindActionCreators?
There was a problem hiding this comment.
You can pass an object as second argument instead of mapDispatchToProps. It’s a documented shortcut and has the same effect 😉
|
This looks really great. For testing components I would suggest we start using Enzyme (but if you decide to add it please make sure to leave a note in #1481). |
|
thanks Dan. I just left a comment there to say I would like to try adding it here only. I've never used it, but it looks great. Will start working on this tonight. |
|
Actually there’s no rush: I’d rather see enzyme as a separate PR, and merge this sooner even if this means we drop component tests temporarily. Anything left to do here to bring it to mergeable state? I see travis is failing. |
|
Ok, sounds good. The linter was failing on test files earlier. Made the changes we discussed above, using I've tested it out locally and it looks good. As long as travis passes I would say this is ready to merge! |
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "redux-todomvc-example", | |||
| "version": "0.0.0", | |||
| "description": "Redux TodoMVC example", | |||
| "description": "Redux Todos example", | |||
|
Thanks, fixed those above items. One thing I did notice, when I inspect in chrome devtools I am seeing a lot of noise that looks like from sourcemaps? Any idea how to disable those messages?
|
|
So it looks like changing webpack |
|
Try |
|
good call! fixed |
|
should we update dependencies as well?
|
|
Ping, ready to merge. |
|
I have enzyme tests for the components. Should I include them in this PR? |
|
Can you squash some of these commits into more manageable chunks? |
add initial state don't hardcode todo id add key to Link items rename component to Header move Footer into component rename Footer container handle show/hide of clear completed button use bindActionCreators for brevity enable hot reloading of reducers add constants fix warnings import order use uppercase for consistency
fix package name move todomvc-app-css to dep add missing dep fix description and move dep add enzyme tests
mwilc0x
force-pushed
the
update-todomvc-example
branch
from
dcf6d51
to
a8daa28
Compare
|
Rebased and squashed. Can you please review and let me know if this is ok? |
|
Dan redid all the examples on create-react-app, so this probably needs a major update. I'm going to close for now but you can resubmit if you have the time to refactor everything. Sorry for the hassle! |
|
Cool, no worries :) |
Hi @gaearon,
This is my attempt at #1354.
I basically took the
todosexample and built off of that, adding in some of the existingtodomvcexample code.Things I did:
The only thing I'm not sure about is testing components. Is there any way you would suggest for this?
I'd love to hear your feedback! I'm happy to make any changes required.
fixes #1354
Cheers