Skip to content

London | 25-ITP-May | Halimatou Saddiyaa | Sprint 3 | Todo list #765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Halimatou-saddiyaa
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

I've implemented the Todo list app with the following requirements:

  • display the initial list of todos
  • each todo has a delete and tick icon
  • can add a new todo to the list
  • can strike through a todo when it is completed
  • can undo a strikethrough on a todo
  • can delete a todo from the list
  • can remove all completed todos

Please review my PR.

Questions

Ask any questions you have for your reviewer.

@Halimatou-saddiyaa Halimatou-saddiyaa added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 13, 2025
@sambabib sambabib 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 Aug 14, 2025
/>
</div>
<div>
<button type="submit" class="btn btn-primary mt-2">Add Todo</button>

Choose a reason for hiding this comment

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

small design tip: if you have two buttons side by side (especially buttons with different functions), make sure to distinguish them by colour.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've changed the color of one of the button.

return;
}

if (newTask !== "") {

Choose a reason for hiding this comment

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

solid implementation. side note: when you run checks like this, you should also run a check for the opposite scenario. for example, when newTask is empty, the add todo button should give feedback that the input field is empty but I understand this is outside the scope of this assignment. it is just something for you to note moving forward.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll keep it in mind for my next projects.

Choose a reason for hiding this comment

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

you can give it a try right now. when the button is clicked but the input field is empty, display a simple message telling the user to write something.

Choose a reason for hiding this comment

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

you are almost there.

Copy link
Author

Choose a reason for hiding this comment

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

I’ve added a message that alerts the user if they try to submit an empty input field.

trashIcon.setAttribute("aria-hidden", "true");
trashIcon.addEventListener("click", () => {
const index = todos.indexOf(todo);
if (index > -1) {

Choose a reason for hiding this comment

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

this is an okay solution but there is a better way to do this. have you explored other array methods?

Copy link
Author

Choose a reason for hiding this comment

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

I've replaced this by an array filtering method inside a function

Choose a reason for hiding this comment

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

better solution. well done!

@@ -1,25 +1,73 @@
function populateTodoList(todos) {

Choose a reason for hiding this comment

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

this function handles both data updates and UI changes, it works fine for a small task like this but when you work on much larger projects, you want to be able to reuse certain functions. in that case, keeping your code as modular as possible is the recommended approach.

Copy link
Author

Choose a reason for hiding this comment

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

I've refactored my code to create other functions to handle data updates and UI changes

@sambabib sambabib added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 15, 2025
@Halimatou-saddiyaa
Copy link
Author

Thank you for reviewing my PR @sambabib
I've made the necessary changes, can you please check again?

@Halimatou-saddiyaa Halimatou-saddiyaa added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 15, 2025
todo.completed = !todo.completed;
}

function removeTodo(todos, todo) {

Choose a reason for hiding this comment

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

very good. it's much cleaner and reusable.

@sambabib sambabib added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 15, 2025
@Halimatou-saddiyaa
Copy link
Author

@sambabib I've changed my code to implement the suggested corrections.
Can you please check again?

@Halimatou-saddiyaa Halimatou-saddiyaa added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 3 Assigned during Sprint 3 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants