Week4 #40
Week4 #40
Conversation
Week1 version1
todo-app/src/TodoItem.js
Outdated
| import './index.css'; | ||
|
|
||
|
|
||
| @observer |
There was a problem hiding this comment.
Is it necessary to make the TodoItem component a MobX-observer? TodoItem is not getting its data directly from the store, only from its props, so you don't need to do this.
There was a problem hiding this comment.
I think I need that, 'deleteTodo' and 'changeStatus' do not functioning otherwise.
There was a problem hiding this comment.
See #38 and Ji's detailed explanation here.
todo-app/src/TodoList.js
Outdated
| import {observer} from 'mobx-react'; | ||
| import {todoStore} from './stores'; | ||
|
|
||
| @observer |
There was a problem hiding this comment.
Don't forget: in the recent version of mobx-react you need to also add @inject.
todo-app/src/TodoList.js
Outdated
| }; | ||
|
|
||
| onDelete = (todoID) => { | ||
| todoStore.onDelete(todoID); |
There was a problem hiding this comment.
Small thing: todoStore.onDelete is not an event handler, but a method on todoStore. Best to just call it delete or deleteTodo.
todo-app/src/TodoList.js
Outdated
| } | ||
|
|
||
| onDone = (todoID) => { | ||
| todoStore.onDone(todoID); |
| </div> | ||
| <ul> | ||
| {todoStore.todos.map((todoItem, i) => ( | ||
| <React.Fragment key={todoItem.id}> |
There was a problem hiding this comment.
No need to use a Fragment here, as there's only one child. You can just render the <TodoItem> directly.
todo-app/src/stores/TodoStore.js
Outdated
| } | ||
|
|
||
| @action | ||
| onDelete(todoID){ |
There was a problem hiding this comment.
See comment on naming above.
todo-app/src/stores/TodoStore.js
Outdated
| } | ||
|
|
||
| @action | ||
| onDone(todoID){ |
There was a problem hiding this comment.
See comment on naming above.
todo-app/src/stores/TodoStore.js
Outdated
| this.todos.push(todo); | ||
| } | ||
|
|
||
| @action |
There was a problem hiding this comment.
Great! This is an action method, and you're correct to mark it as such.
|
I have changed some after your comments. |
Sorry for late. I had very busy week.