Birmingham | 26-ITP-May | Gabriel Pawuoi | Sprint 3 | Implement todo-list - #1416
Birmingham | 26-ITP-May | Gabriel Pawuoi | Sprint 3 | Implement todo-list#1416KhotKeys wants to merge 2 commits into
Conversation
KhotKeys
added
🏕 Priority Mandatory
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
There don't appear to be any changes listed in the "files changed" tab - what have you added in this PR?
LonMcGregor
added
Reviewed
42abea3 to
ccfb60d
Compare
Apologies for that! My local branch was misconfigured during the initial push, which caused the implementation files to get left behind. I have cleanly reset the branch history and force-pushed the complete todo-list solution folder now. Please let me know your thoughts! |
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
There's a lot here, probably more than is needed for this task. Did you write all this yourself?
I ran the page through the lighthouse check as it seems quite complex, and it failed the accessibility check - can you figure out why?
Also, the todolist works well, but I can't see which of the features you added that the readme asked about, can you talk me through it?
a78b5bc to
8887c0a
Compare
Thanks for the feedback! The accessibility issue was that the complete and delete buttons were icon only with no accessible label, so screen readers couldn't describe them. I've added aria-label attributes to both buttons to fix that. The main feature the README asked for is the "Delete completed tasks" button, which I've now added. It calls a deleteCompleted() function in todos.mjs that removes all completed tasks at once. I also wrote Jest tests for it covering three cases: removing completed tasks, doing nothing when none are completed, and emptying the list when all are completed. I also fixed a small CSS bug where the icon colors weren't applying because the selectors were targeting tags, but the HTML uses |
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work so far, one minor thing to fix.
I would suggest avoiding force pushing, developers tend to recommend against doing that as it can cause problems if you are working across multiple machines / in a team.
| // Set up tasks to be performed once on page load | ||
| window.addEventListener("load", () => { | ||
| document.getElementById("add-task-btn").addEventListener("click", addNewTodo); | ||
| document.getElementById("delete-completed-btn").addEventListener("click", () => { |
There was a problem hiding this comment.
Compare this with the event listener above, do you see the difference in code style. Can you make them more consistent?
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great, I would say this is complete now!
LonMcGregor
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Implemented addTask to append a new task object to the todos array, deleteTask to remove a task by index if it exists, and toggleCompletedOnTask to flip the completed boolean on a valid task.
All three functions handle out-of-range indices gracefully without throwing errors or modifying the array.