London | 26-ITP-May | Edina Kurdi | Sprint 3 | Todo List - #1429
edinakurdi wants to merge 13 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
edinakurdi
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
cjyuan
changed the title
cjyuan
added
the
Needs Review
edinakurdi
added
🏕 Priority Mandatory
hackertainment
added
Review in progress
|
Good try, and keep it up :-) |
hackertainment
added
Reviewed
Thank you. Can you please provide some comments as to what to improve on? |
Oh, I commented directly with the corresponding lines of code. Two fixes required as shown in above. Thank you. |
…o move to on page load (windwo.addEventListener(load,..)
|
From what I can see, your code looks fairly clean and tidy. So I have no further suggestion on the coding part itself. Just I would be interested with your development/thinking process. Maybe when you have time in the future, try to book a pair-programming section with a mentor, so as to add the implementation of the optional deadline feature into this exercise. :-) |
hackertainment
added
Complete
Oh, I was going back and forth - got myself very confused with the code! Didnt realise that this was accepted in the meanwhile. Soulds like a good idea about the deadline feature, ill try and do that! Thank you. |
| document | ||
| .getElementById("delete-completed-btn") | ||
| .addEventListener("click", () => { | ||
| Todos.deleteCompleted(todos); | ||
| render(); | ||
| }); |
There was a problem hiding this comment.
Even the listener is added at the end of the script, there is no guarentee the component in HTML would be ready. So with reference to the existing "Add" button as an example, when/where would be better for adding your event listener instead?
| // 2. In `todos.mjs`, implement a function `deleteCompleted(todoList)` that removes all completed | ||
| // ToDos from the given list. | ||
| export function deleteCompleted(todoList) { | ||
| for (let i = todoList.length - 1; i >= 0; i--) { | ||
| if (todoList[i].completed === true) { | ||
| todoList.splice(i, 1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This would better be moved to script.js and call the Todo.deleteTask() instead of using splice() inside your fundtion.
3 participants
Learners, PR Template
Self checklist
Changelist