Skip to content

LONDON | 25-ITP-25 | TEWODROS BEKERE | SPRINT-3 | ToDo-List #735

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.

Sign up for GitHub

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 1 commit into
base: main
Choose a base branch
from
+276 −3
Open

LONDON | 25-ITP-25 | TEWODROS BEKERE | SPRINT-3 | ToDo-List #735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions Sprint-3/todo-list/index.html
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

  • I see a few typos where an extra space has been added before the title of an element or a CSS class name. Can you review your file and fix these typos?

  • 💡 Are you using VSCode? If yes, you can right-click within the code, and choose 'Format Document'. This will align your code nicely and make it easier to read!

Copy link

Choose a reason for hiding this comment

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

While running the Lighthouse accessibility report, a few issues came up that should be fixed:

  • The background and foreground colours chosen on the button don't have a high enough contrast ratio, making it hard to read for some people.
  • The <meta name="viewport"> tag is missing.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<!DOCTYPE html>
<!--<!DOCTYPE html>
Copy link

Choose a reason for hiding this comment

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

It's best to completely remove the commented-out code from the file.

<html lang="en">
<head>
<meta charset="UTF-8" />
Expand All @@ -8,14 +8,15 @@
</head>
<body>
<form>
<ul id="todo-list"></ul>
<div>
<input type="text" placeholder="New todo..." />
</div>
<div>
<button type="submit">Add Todo</button>
<button type="submit" id="todoInput" />Add Todo</button>
<button
type="button"
id="remove-all-completed"
id="remove-completed"
class="btn btn-primary mb-3"
>
Remove all completed
Expand All @@ -24,4 +25,28 @@
</form>
<script src="script.js"></script>
</body>
</html>-->
Copy link

Choose a reason for hiding this comment

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

The --> appears on the UI of your application. Can you adjust your html file so this isn't visible?

Image


<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<title>Todo List</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>
<div class=" container">
<h1> Todo List</h1>
<form class=""add-todo-form>
Copy link

Choose a reason for hiding this comment

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

At line 40, there is a small typo. Can you find it and fix?

<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>
<ul id="todo-list"></ul>

</div>

<script src="script.js"></script>
</body>
</html>

111 changes: 111 additions & 0 deletions Sprint-3/todo-list/script.js
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Your JS code is written well and I'm able to interact with your to-do list with ease. I have a items for your to review and implement in order to make your code more robust.

  • Replace all let with const (as long as the variable is not being reassigned any values).
  • Format the javascript code to be easier to read (right-click -> Format Document).

Original file line number Diff line number Diff line change
@@ -1,8 +1,41 @@
function populateTodoList(todos) {
let list = document.getElementById("todo-list");
// Write your code to create todo list elements with completed and delete buttons here, all todos should display inside the "todo-list" element.
list.innerHTML = ""; // clears the available list
if (todos.length === 0) {
Copy link

Choose a reason for hiding this comment

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

Nice example of an early return at lines 5-8. 👍

list.innerHTML = `<li class="empty-state">No todos yet. Add one above!</li>`;
return;
}
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";

let todoText = document.createElement("span");
todoText.className = todo.completed ? "todo-text completed" : "todo-text";
todoText.textContent = todo.task;

let completeButton = document.createElement("button"); // create complete button
completeButton.className = todo.completed
? "complete-btn completed"
: "complete-btn";
completeButton.textContent = todo.completed ? "Undo" : "Complete";
completeButton.onclick = () => toggleComplete(index);

let deleteButton = document.createElement("button"); // create delete button
deleteButton.className = "delete-btn";
deleteButton.textContent = "Delete";
deleteButton.onclick = () => deleteTodo(index);

listItem.appendChild(todoText); // append elements to list item
listItem.appendChild(completeButton);
listItem.appendChild(deleteButton);

list.appendChild(listItem); // append list item to todo list
});
}


// These are the same todos that currently display in the HTML
// You will want to remove the ones in the current HTML after you have created them using JavaScript
let todos = [
Expand All @@ -12,14 +45,92 @@ let todos = [

populateTodoList(todos);

function toggleComplete(index) {
// function to toggle complete status
todos[index].completed = !todos[index].completed;
populateTodoList(todos);
}

function deleteTodo(index) {
//function to delete specific todo
todos.splice(index, 1);
populateTodoList(todos);
}
populateTodoList(todos);
Copy link

Choose a reason for hiding this comment

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

🤔 Is this populateTodoList a repetition of the same thing from line 46?

// This function will take the value of the input field and add it as a new todo to the bottom of the todo list. These new todos will need the completed and delete buttons adding like normal.
function addNewTodo(event) {
// The code below prevents the page from refreshing when we click the 'Add Todo' button.
event.preventDefault();
// Write your code here... and remember to reset the input field to be blank after creating a todo!
let input =
Copy link

Choose a reason for hiding this comment

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

At line 65-68, can you explain why you are looking for 3 different things for input?

document.getElementById("new-todo-input") ||
document.getElementById("todo-input") ||
document.querySelector('input[type="text"]');

let todoText = input.value.trim();

if (todoText === "") {
alert("Please enter a todo item!");
return;
}
let newTodo = {
// create new todo object
task: todoText,
completed: false,
};

todos.push(newTodo); // ad todos array

input.value = ""; //clear input field

populateTodoList(todos); //refresh the todo list
}


// Advanced challenge: Write a fucntion that checks the todos in the todo list and deletes the completed ones (we can check which ones are completed by seeing if they have the line-through styling applied or not).
function deleteAllCompletedTodos() {
// Write your code here...
todos = todos.filter((todo) => !todo.completed); // filter out the completed todos

populateTodoList(todos); // refresh the display

console.log("The completed todos are deleted!");
}
// Event listener
document.addEventListener("DOMContentLoaded", function () {
// Try to find the form by different selectors
Copy link

Choose a reason for hiding this comment

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

Why do you find the form via different selectors instead of using one selector?

let form =
document.querySelector("form") ||
document.querySelector(".add-todo-form") ||
document.getElementById("todo-form");

if (form) {
form.addEventListener("submit", addNewTodo);
} else {
Copy link

Choose a reason for hiding this comment

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

🤔 I think the console log can be removed here as it won't serve a purpose to the user since they won't be able to act upon your console log message.
(Your console log is a good idea once an application is in a Production environment and logging is needed to troubleshoot application problems.)

console.log(
"No form found. Make sure your HTML has a form element or call addNewTodo() directly from a button onclick."
);

// Try to find an add button and attach the event
let addButton =
document.querySelector('button[type="submit"]') ||
document.querySelector(".add-btn") ||
document.getElementById("add-todo-btn");

if (addButton) {
addButton.addEventListener("click", addNewTodo);
}
}

// Try to find delete all completed button
let deleteAllBtn =
Copy link

Choose a reason for hiding this comment

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

If there isn't an item in the list, can you think of a way to disable the 'Delete All Complete' button. Ideally, the user shouldn't be able to click on the button if there aren't any items to delete!

document.querySelector(".delete-all-btn") ||
document.getElementById("delete-all-btn");

if (deleteAllBtn) {
deleteAllBtn.addEventListener("click", deleteAllCompletedTodos);
}
});



137 changes: 137 additions & 0 deletions Sprint-3/todo-list/style.css
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

This is only a suggestion. You do not need to implement for this PR.

💡Since you are using a lot of different blues and greens within your CSS file, it might be easier to maintain them by defining them at the top of the file. For example:

:root {
  --primary-blue: #3936f4;
  --primary-green: #085a13;
  --accent-red: #dd1024;
}

Then use var(--primary-blue) etc. within your CSS selector definitions.

Original file line number Diff line number Diff line change
@@ -1 +1,138 @@
body {
font-family: Arial, sans-serif;
max-width: 600px;
margin: 50px auto;
padding: 20px;
background-color: #f5f5f5;
}

.container {
background: white;
padding: 30px;
border-radius: 10px;
box-shadow: 0 2px 10px rgba(0, 0, 0, 0.1);
}

h1 {
text-align: center;
color: #333;
margin-bottom: 30px;
}

.add-todo-form {
display: flex;
gap: 10px;
margin-bottom: 30px;
}

#new-todo-input {
flex: 1;
padding: 12px;
border: 2px solid #ddd;
border-radius: 5px;
font-size: 16px;
}

#new-todo-input:focus {
outline: none;
border-color: #4CAF50;
}

button {
padding: 12px 20px;
border: none;
border-radius: 5px;
cursor: pointer;
font-size: 16px;
transition: background-color 0.3s;
}

.add-btn {
background-color: #3936f4;
color: white;
}

.add-btn:hover {
background-color: #085a13;
}

.delete-all-btn {
background-color: #3936f4;
color: white;
margin-bottom: 20px;
margin-top: 30px;
margin-left: 100px;
}

.delete-all-btn:hover {
background-color: #dd1024;
}

#todo-list {
list-style: none;
padding: 0;
}

.todo-item {
display: flex;
align-items: center;
gap: 10px;
padding: 15px;
margin: 10px 0;
background-color: #f9f9f9;
border-radius: 5px;
border-left: 4px solid #547f56;
}

.todo-item.completed {
border-left-color: #ccc;
background-color: #f0f0f0;
}

.todo-text {
flex: 1;
font-size: 16px;
transition: all 0.3s;
}

.todo-text.completed {
text-decoration: line-through;
color: #888;
}

.complete-btn {
background-color: #2196F3;
color: white;
padding: 8px 12px;
font-size: 14px;
}

.complete-btn:hover {
background-color: #1976D2;
}

.complete-btn.completed {
background-color: #FF9800;
}

.complete-btn.completed:hover {
background-color: #F57C00;
}

.delete-btn {
background-color: #f44336;
color: white;
padding: 8px 12px;
font-size: 14px;
}

.delete-btn:hover {
background-color: #da190b;
}

.empty-state {
Copy link

Choose a reason for hiding this comment

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

Can you verify if .empty-state is needed?

text-align: center;
color: #888;
font-style: italic;
padding: 40px;
}