Problem
Hello I don’t have any specific problem but I’m just interested how someone who is experienced in JavaScript would write this small toy program. it’s a simple Todo List app, there are many approaches to writing something like this but I’m not sure if the way I wrote it is the most optimal.
I want to see how others would write this, and maybe learn something new.
I’m not very experienced programmer so any criticism is welcome 🙂
const TodoList = document.getElementById("TodoList");
const AddTodo = document.getElementById("AddTodo");
let ToggleBtns = []; let RemoveBtns = [];
let MyTodos = [
{todoText: "hello world", Completed: true},
{todoText: "hello world", Completed: false},
{todoText: "hello world", Completed: false}
];
function CreateItems(text, index){
const TodoItem = document.createElement("li");
const ToggleButton = document.createElement("button");
const TodoText = document.createElement("p");
const RemoveButton = document.createElement("button");
TodoItem.className = "TodoItem";
ToggleButton.className = "ToggleButton";
TodoText.ClassName = "TodoText";
RemoveButton.className = "RemoveButton";
TodoText.innerText = text;
TodoText.style.textDecoration = (MyTodos[index].Completed === true ? "line-through" : "none");
TodoItem.appendChild(ToggleButton);
TodoItem.appendChild(TodoText);
TodoItem.appendChild(RemoveButton);
TodoList.appendChild(TodoItem);
ToggleBtns.push(ToggleButton);
RemoveBtns.push(RemoveButton);
};
for (i in MyTodos) { CreateItems(MyTodos[i].todoText, i); }
AddTodo.addEventListener("keypress", function(e){
if(e.keyCode === 13){
MyTodos.push({todoText: AddTodo.value, Completed: false});
CreateItems(MyTodos[MyTodos.length -1].todoText, (MyTodos.length -1))
}
});
document.addEventListener("click", function(e){
if(e.target.className === "ToggleButton"){
ToggleIndex = ToggleBtns.indexOf(e.target);
MyTodos[ToggleIndex].Completed =
(MyTodos[ToggleIndex].Completed === true ? false : true);
e.target.parentElement.children[1].style.textDecoration = (MyTodos[ToggleIndex].Completed === true ? "line-through" : "none");
}
else if (e.target.className === "RemoveButton"){
RemoveIndex = RemoveBtns.indexOf(e.target);
if(RemoveIndex !== 0){
MyTodos.splice(RemoveIndex, RemoveIndex);
ToggleBtns.splice(RemoveIndex, RemoveIndex);
RemoveBtns.splice(RemoveIndex, RemoveIndex);
} else {
MyTodos.splice(0, 1);
ToggleBtns.splice(0, 1);
RemoveBtns.splice(0, 1);
}
e.target.parentElement.remove();
}
});
#TodoList {
display: flex;
flex-direction: column;
padding: 0 ;
}
.TodoItem {
display: flex;
flex-direction: row;
}
.TodoItem button {
width: 50px;
}
.TodoItem p {
width: 300px;
text-align: center;
}
#Todos {
display: table;
margin: 0 auto;
}
#AddTodo {
display: table;
height:35px;
width: 70%;
padding: 0 20px;
margin: 0 auto;
}
<div id="Todos">
<input type="text" name="" id="AddTodo">
<ul id="TodoList"></ul>
</div>
Solution
If you had not written this code but were trying to use it, how would you know that the square on either side was a button or what the function of that button was? Buttons need labels and they are generally oval shaped.
When you design or write code for the web you need to consider the user and how they will be using the tool. Even a toy application should follow some basic standards.
Your code works, so congratulations for that and for the effort!
I’m not that experienced myself but after a quick look at your code I notice two important things:
- naming you variables/functions can be improved. Trust me, this is usually a lot harder than it seems.
- casing. In javascript we normally use
camelCasing
, example:helloWorld, stackOverflow, myTodos, youNameIt
- your code is too complicated. Simplifying code is harder than it seems.
Fortunately both things require just experience and patience.
I took the freedom to name some of the variable and to restructure the code a bit and I came up with this:
// here we keep a reference to our DOM elements
const app = document.getElementById('app');
const todoList = document.getElementById('todoList');
const newTodo = document.getElementById('newTodo');
let myTodos = [
{todoText: "hello world", Completed: true},
{todoText: "hello world", Completed: false},
{todoText: "hello world", Completed: false}
];
// this renders the todo items
newTodo.addEventListener('keypress', onKeyPress)
function onKeyPress(e) {
if(newTodo.value && e.code === 'Enter') {
const brandNewTodo = {
todoText: newTodo.value,
Completed: false
}
createTodoElement(brandNewTodo);
newTodo.value = '';
}
}
function createTodoElement(todo) {
const todoContainer = document.createElement('li');
// it makes more sense to have a checkbox for a todo
const todoCheckInput = document.createElement('input');
const todoDeleteBtn = document.createElement('button');
const todoText = document.createElement('span');
todoText.innerText = todo.todoText;
todoContainer.className = 'todo-item ';
todoCheckInput.className = 'btn-check';
todoCheckInput.setAttribute('type', 'checkbox');
// initial checkbox state
todoCheckInput.checked = todo.Completed;
// if initial state of the todo is Completed then we add the css
// class completed
todoText.className =`${todo.Completed && 'completed'}`;
todoDeleteBtn.className = 'btn-delete';
todoDeleteBtn.innerText = 'delete';
// on click event listener to the checkbox
todoCheckInput.addEventListener('click', (e) => {
todo.Completed = !todo.Completed;
todoText.classList.toggle('completed');
})
// on click event listener for removing the todo
todoDeleteBtn.addEventListener('click', (e) => {
e.currentTarget.parentNode.remove();
})
todoContainer.appendChild(todoCheckInput);
todoContainer.appendChild(todoText);
todoContainer.appendChild(todoDeleteBtn);
todoList.appendChild(todoContainer);
}
That’s it for the js.
As you can see, there are no big changes but I think it’s easier to read and maintain.
Here is the html:
<div id="app">
<input type="text" name="newTodo" id="newTodo" placeholder="create new todo and press ENTER" class="new-todo">
<ul id="todoList" class="todo-list"></ul>
</div>
<script src="app.js"></script>
and here is the CSS:
#app {
max-width: 600px;
}
.new-todo {
width: 100%;
}
.todo-list {
width: 100%;
padding: 0;
}
.todo-item {
display: flex;
justify-content: space-between;
padding-bottom: 0.3rem;
}
.completed {
text-decoration: line-through;
}
Good luck and keep up the good work!
Here is a js fiddle link: https://jsfiddle.net/c103nvf7/