ITP-JAN-LONDON|FOROGH AGHAEIYARIJANI|MODULE-DATA-GROUP|WEEK3|SPRINT3|TO-DO-LIST|Create ToDos #477
Conversation
Forogh-Aghaeiyarijani
added
the
Needs Review
cjyuan
added
Reviewed
|
Resolved |
Sprint-3/todo-list/index.html
Outdated
There was a problem hiding this comment.
Did these two icons checkIcon and trashIcon appear in your browser when you test this app?
I asked ChatGPT which CSS library most likely have those classes defined, and it suggested these:
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet">
After I included the above code in index.html, two icons show up for each to do item.
There was a problem hiding this comment.
todos array is the application state.
All the code to render the list based on todos should be defined in populateTodoList() (but not in the call back functions assigned to checkIcon and trashIcon).
Sprint-3/todo-list/script.js
Outdated
| checkIcon.style.cursor = "pointer"; | ||
| checkIcon.addEventListener("click", () => { | ||
| li.style.textDecoration = | ||
| li.style.textDecoration === "line-through" ? "none" : "line-through"; |
There was a problem hiding this comment.
The code to apply line-through should be place outside of this callback function.
Inside this callback function, focus on updating the application states, then call populateTodoList() to update the view in the following manner:
checkIcon.addEventListener("click", () => {
// Change the "completed" property of the current task
populateTodoList(todos); // update view
});
Sprint-3/todo-list/script.js
Outdated
| let trashIcon = document.createElement("i"); | ||
| trashIcon.className = "fa fa-trash"; | ||
| trashIcon.style.cursor = "pointer"; | ||
| trashIcon.addEventListener("click", () => { | ||
| li.remove(); | ||
| }); |
There was a problem hiding this comment.
Removing the DOM element does not affect the application states. In other words, the statement at line 27 does not delete the corresponding element in array todos. As a result, when a new todo task is added after some items were deleted, the previously "deleted" items will reappear.
Instead, you should delete the corresponding item in the array todos, then call populateTodoList(todos) to update the view.
Sprint-3/todo-list/script.js
Outdated
| todos = todos.filter((todo) => { | ||
| return ![...list.children].some((li) => li.textContent.includes(todo.task)); | ||
| }); |
There was a problem hiding this comment.
What would happened if two todo tasks have identical task property value but only one of the tasks is completed?
Why not filter todos based on the completed property of the element?
And after you updated todos, just call populateTodoList(todos) to update the view. You don't need the code at lines 69-77.
Forogh-Aghaeiyarijani
added
the
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.