Update TodoMVC example to be more in line with other examples #2757
Conversation
tomipaul
commented
Dec 15, 2017
tomipaul
commented
- Store visibilityFilter in the state
- Update presentational components
- Add container components
- Update reducer code to use composition
- Add Reselect
|
Sorry I haven't had a chance to look at this. It's fairly extensive, so it may take some time. If others want to make some review comments, go right ahead. I'm not the sole gate keeper for contributions to this project 😄 |
| return ( | ||
| <section className="main"> | ||
| { | ||
| (todosCount > 0) ? |
There was a problem hiding this comment.
How about doing !!todosCount && <Component ... /> instead of the ternary? Would also get rid of the null being returned if the condition is not met.
| import VisibleTodoList from '../containers/VisibleTodoList' | ||
|
|
||
| const MainSection = (props) => { | ||
| const { todosCount, completedCount, actions } = props |
There was a problem hiding this comment.
I would just destructure these inside the parameters of the function and have an implicit return.
| } | ||
| <VisibleTodoList /> | ||
| { | ||
| (todosCount > 0) ? |
| )} | ||
| </ul> | ||
| { | ||
| (completedCount > 0) ? |
There was a problem hiding this comment.
Ditto for the ternary above.
| @@ -4,5 +4,6 @@ export const addTodo = text => ({ type: types.ADD_TODO, text }) | |||
| export const deleteTodo = id => ({ type: types.DELETE_TODO, id }) | |||
| export const editTodo = (id, text) => ({ type: types.EDIT_TODO, id, text }) | |||
| export const completeTodo = id => ({ type: types.COMPLETE_TODO, id }) | |||
| export const completeAll = () => ({ type: types.COMPLETE_ALL }) | |||
| export const completeAllTodos = () => ({ type: types.COMPLETE_ALL_TODOS }) | |||
There was a problem hiding this comment.
Why the rename? Seems like the context is obvious here regardless.
There was a problem hiding this comment.
Yes, it is obvious. I just feel having Todos attached to the completeAll will make it much more explicit and consistent with the others.
| import classnames from 'classnames' | ||
|
|
||
| const Link = ({ active, children, setFilter }) => { | ||
| return ( |
There was a problem hiding this comment.
Use an implicit return. No need for the block.
| mapDispatchToProps | ||
| )(Link) | ||
|
|
||
| export default FilterLink |
There was a problem hiding this comment.
I would prefer doing
export default connect(
mapStateToProps,
mapDispatchToProps
)(Link)
But I'm not sure if this is a convention or not, I just don't see the point in the intermediary variable that is disposable anyway.
| addTodo: PropTypes.func.isRequired | ||
| } | ||
|
|
||
| export default connect(null, { addTodo })(Header) |
There was a problem hiding this comment.
Even more reasons to use the convention described above then if this is already in use here.
| import { getCompletedTodoCount } from '../selectors' | ||
|
|
||
|
|
||
| const mapStateToProps = (state) => { |
There was a problem hiding this comment.
Seems like in some cases you're using an implicit return for an object and in some cases an explicit one. I would recommend keeping it consistent and using an implicit return in all cases then, unless you require a block for extra statements.
|
@tomipaul Any chance you can address those comments and also resolve the merge conflicts? |
|
Oops Somehow I missed your comment @timdorr Back when the comments were made, I started addressing them before some other work took me off. |
tomipaul
force-pushed
the
update-todo-mvc
branch
from
d4d6cd4
to
5299d1a
Compare
Add action to set visibility filter Change COMPLETE_ALL to COMPLETE_ALL_TODOS
handle set_visibility_filter action store visibilityFilter in the state update complete_all to complete_all_todos update reducer code to use composition
Add a root App component Add a link component to select visibility filter Add a todolist component to display todos Refactor mainsection and footer to functional components
add containers for link, header, mainsection and todolist add Reselect
tomipaul
force-pushed
the
update-todo-mvc
branch
from
5299d1a
to
98e2102
Compare
|
@timdorr I'm done with the fixes. |
|
That should be fixed in master now too. Waiting on a build to confirm that and then I'll merge this in. Thanks for keeping up with this! |
|
Thanks, it was fun all along. |
