Skip to content

Implementing to do list#1

Open
pjuhel wants to merge 5 commits into
mainfrom
ImplementingToDoList
Open

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

Conversation

@pjuhel

@pjuhel pjuhel commented Mar 14, 2021

Copy link
Copy Markdown
Owner

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.

@louis-bompart louis-bompart left a comment

Copy link
Copy Markdown
Collaborator

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 thread my-app/src/App.tsx
Comment on lines +23 to +29
if (todo === selectedTodo) {
return {
...todo,
complete: !todo.complete,
};
}
return todo;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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