Skip to content

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | ToDo List App#1136

Closed
mahmoudshaabo1984 wants to merge 1 commit into
CodeYourFuture:mainfrom
mahmoudshaabo1984:todo-list-app
Closed

Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | ToDo List App#1136
mahmoudshaabo1984 wants to merge 1 commit into
CodeYourFuture:mainfrom
mahmoudshaabo1984:todo-list-app

Conversation

@mahmoudshaabo1984
Copy link
Copy Markdown

Hello Reviewers / CodeYourFuture Team,

I have completed the ToDo List App project for Sprint 3. I am excited to share that I have successfully implemented the mandatory requirement for the mass delete feature.

🎯 Acceptance Criteria Met:

  • UI Update: Added a "Delete completed tasks" button to index.html with the correct ID.
  • Logic Implementation: Created the deleteCompleted(todoList) function in todos.mjs to remove all completed tasks.
  • Testing: Wrote passing Jest tests in todos.test.mjs to verify the logic works correctly (including edge cases).
  • Integration: Connected the new button to the deleteCompletedTodos function in script.mjs and successfully updated the DOM.

💻 Technical Implementation Details:

  • Utilized a reverse for loop in the deleteCompleted function. This approach ensures that modifying the array length during iteration does not cause index shifting or skip any items, which is a common issue when using splice() in a standard forward loop.
  • Ensured all modular imports/exports work correctly as ES Modules.
  • All Jest tests (npm test) are passing successfully.

Thank you for your time and feedback!

Best regards,
Mahmoud Shaabo

@mahmoudshaabo1984 mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026
@mahmoudshaabo1984
Copy link
Copy Markdown
Author

Hi @cifarquhar,

As I submit my Pull Request for the ToDo List App, I want to take a moment to thank you for all your efforts and the valuable knowledge you share with us in this course.

For this task, I have successfully implemented the mass delete feature for completed ToDos, making sure the business logic in todos.mjs is correctly separated from the UI in script.mjs. All Jest tests are also passing successfully.

Thank you again for your time and continuous support!

Best regards,
Mahmoud Shaabo

@Luro91 Luro91 added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 6, 2026
Copy link
Copy Markdown

@Luro91 Luro91 left a comment

Choose a reason for hiding this comment

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

Well done. The implementation works as expected

Comment on lines +32 to +37
for (let i = todos.length - 1; i >= 0; i--) {
if (todos[i].completed === true) {
todos.splice(i, 1);
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works correctly. You can also research build in array functions. There is a function that allows you filter elements in an array based on a condition.

@Luro91 Luro91 added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 6, 2026
@illicitonion
Copy link
Copy Markdown
Member

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

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

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants