-
-
Notifications
You must be signed in to change notification settings - Fork 211
London | ITP-May-2025 | Hibo Sharif | Sprint 3 | Todo_list #702
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
Conversation
|
Really great submission! Very well organised and hitting the brief well - a few comments to take the code to the next level! |
Sprint-3/todo-list/index.html
Outdated
|
|
||
| <div class=" container"> | ||
| <h1> Todo List</h1> | ||
| <form class=""add-todo-form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small issue on the form element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @MattGoodwin0 . I have made the change.
Sprint-3/todo-list/index.html
Outdated
| <input type="text" id="new-todo-input" placeholder="New todo..." /> | ||
| <button type="submit" class="add-btn">Add Todo</button> | ||
| </form> | ||
| <button onclick="deleteAllCompletedTodos()" class="delete-all-btn">Delete All Completed</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two button implement their interaction functionality differently - is there a reason for this?
One uses the class and the other onclick.
Sprint-3/todo-list/script.js
Outdated
| event.preventDefault(); | ||
| // Write your code here... and remember to reset the input field to be blank after creating a todo! | ||
|
|
||
| let input = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the HTML fix you can probably look to simplify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MattGoodwin0 ,
I have updated the code.
Sprint-3/todo-list/script.js
Outdated
| todos.forEach((todo, index) => { | ||
| // create the todo elements | ||
| const listItem = document.createElement("li"); // create list item | ||
| listItem.className = todo.completed ? "todo-item completed" : "todo-item"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a way we could reduce this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a compound selector be used here to reduce the javascript code.
Let me know if this isn't part of the sprint!
Sprint-3/todo-list/script.js
Outdated
| listItem.className = todo.completed ? "todo-item completed" : "todo-item"; | ||
|
|
||
| let todoText = document.createElement("span"); | ||
| todoText.className = todo.completed ? "todo-text completed" : "todo-text"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above - Can you think of a way we could reduce this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a compound selector be used here to reduce the javascript code.
Let me know if this isn't part of the sprint!
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Sprint part (Module-Data-Group) doesn't match expected format (example: 'Sprint 2', without quotes) |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Module-Data-Group) doesn't match expected format (example: 'Sprint 2', without quotes) |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s |
Learners, PR Template
Self checklist
Changelist
Questions
Ask any questions you have for your reviewer.