Skip to content

Implementing to do list#1

Open
pjuhel wants to merge 5 commits intomainfrom
ImplementingToDoList
Open

Implementing to do list#1
pjuhel wants to merge 5 commits intomainfrom
ImplementingToDoList

Conversation

@pjuhel
Copy link
Owner

@pjuhel pjuhel commented Mar 14, 2021

First version of the TodoList program. Complete with add/delete functionnalities.

pjuhel added 5 commits March 13, 2021 17:35
Starting fresh to create the todolist
Creating TodoList and TodoListItem component, displaying them in the app with a default state
Basic todolist, ability to add items to an existing list.
Changing syntax to be more newcomer friendly, especially destructuration occurences.
Implement a delete button. Each item in the list now bear a unique Id for identifying purposes.
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, few suggestions to make it even betta

Comment on lines +23 to +29
if (todo === selectedTodo) {
return {
...todo,
complete: !todo.complete,
};
}
return todo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I would recommend the uses of a ternary

Suggested change
if (todo === selectedTodo) {
return {
...todo,
complete: !todo.complete,
};
}
return todo;
return {
...todo,
complete: todo === selectedTodo ? !todo.complete : todo.complete
};

}

export const AddTodoForm: React.FC<Props> = (param) => {
//const { addTodo } = param;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't leave commented code, it's dirty :P
If you leave it for documentation purposes, I'd advise you to put an actual comment with it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shall make a documentation of it. I wanted to keep track of the possible destructuration here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants