4
\$\begingroup\$

I created my first "program": a to-do list. I would appreciate it if somebody could look at the code and give me some hints on how to improve it.

var list = document.createElement("ol");
list.id = "list";
list.className = "list";
		
var addTask = document.getElementById("inputBtn");
addTask.addEventListener('click', function (){
			
	// checking whether the field is empty
	var task = document.getElementById("task").value;
	if(task=="") alert("Are you sure you do not have any task to do?");
	else
	{
		// adding an element
		addElement (list, task);
		
		//clear the input field
		document.getElementById("task").value ="";
	}
}, false);
	
function addElement(ol, text)
{
	var element = document.createElement("li");
	element.id = "element";
	element.className = "element";
	element.appendChild(document.createTextNode(text));
	ol.appendChild(element);
	document.getElementById("board").appendChild(ol);
	
	//adding a remove button
	addRemoveBtn(element);
	
	//adding a function which change the decoration of the text
	addCrossStyle(element);
}

function addRemoveBtn(li)
{
	//creating a remove button
	var removeTask = document.createElement("input");
	removeTask.setAttribute("type", "button");
	removeTask.setAttribute("value", "Remove");
	removeTask.setAttribute("class", "remove");	
	
	//remove task function
	removeTask.addEventListener('click', function (){
		li.parentNode.removeChild(li);
	}, false);
	li.appendChild(removeTask);
}
		
function addCrossStyle(li)
{
	var check = false;
		
		li.addEventListener('click', function (){
			if (check==false)
			{
					li.style.textDecoration = "line-through";
					li.style.textDecorationColor = "red";
					check = true;
			}
			else
			{
					li.style.textDecoration = "none";
					check = false;
			}
		}, false);
}	
body
{
	margin: 0;
	background-color: #26282E;
	color: #b3b3ff;
    font-family: 'Arial', sans-serif;
    font-size: 28px;
}

h1
{
	font-size: 64px;
	font-weight: 400;
	text-align: center;
	letter-spacing: 5px;
	margin-bottom: 0px;
	margin-top: 40px;
}

.addtask
{
	margin-top: 20px;
	text-align: center;
}

input
 {
	  margin-top: 10px;
	  border: solid;
	  padding: 10px;
	  font-size: 20px;
	  background-color: #b3b3ff;
}

input::placeholder 
{
	  color: white;
	  text-align: center;
}

.inputBtn
{
	background-color: #8080ff;
	margin-top: 20px;
	border: 3px black solid;
    color: white;
    padding: 15px 32px;
    text-decoration: none;
    display: inline-block;
	font-size: 28px;
}

.inputBtn:hover
{
	cursor:pointer;
	background-color: #6b6b99;
}

.list
{
	width: 900px;
	margin: auto;
	background-color: #b3b3ff;
	margin-top: 10px;
	color: white;
}

.remove
{
	background-color: #b30000;
	border: 3px black solid;
	color: white;
    padding: 5px 15px;
	font-size: 12px;
	position: absolute;
	margin-right: 5px;
	right: 0;
	top: 0;
	opacity: 0.7;
}
.remove:hover
{
	cursor: pointer;
	opacity: 1;
	background-color: red;
}

.element
{
	position: relative;
	padding: 15px 32px;
	font-size: 22px;
	word-wrap: break-word;
	padding-bottom: 10px;
	border-bottom: solid #8080ff;
	cursor: pointer;
}

.element:hover
{
	background: #8080ff;
}
<!DOCTYPE html>
<html lang="pl">
<head>

	<meta charset="utf-8">
	<title>To Do List</title>
	<meta name="author" content="Szymon Wolny">
	
	<meta http-equiv="X-Ua-Compatible" content="IE=edge,chrome=1">
	
	<link rel="stylesheet" href="todo.css">

</head>

<body>
	<header>
		
		<h1> To Do List</h1>
	</header>
	
	<main>
	
		<article>
			
			<div class="addtask">
				<form>

					<input type ="text" name="task" placeholder="New task..." id="task" size="45" ><br>

				</form>
				<button type="button" id="inputBtn" class="inputBtn">Add a new task to the List </button>
			</div>
			
			<div id="board"></div>
			
		</article>
	
	</main>
	
	<script src="todo.js"></script>
	
</body>
</html>

\$\endgroup\$
0

1 Answer 1

Reset to default
1
\$\begingroup\$

so just 2 things here:

  1. if(task=="") alert("Are you sure you do not have any task to do?"); such comparison is a bit risky in JS. In case of checking for empty values it's better to do if(!condition) {} else {} or at least if(task ==="") (strong type checking). Not saying it's wrong in this case, just a rule of a thumb. Read more about issues with JS comparison / type casting, you'll find out what I mean.
  2. Creating UI in code is fine of course, and it's nice to understand what's going on under the hood, but for readability of your UI you mind find it better to define everything up-front in HTML and than later just show / hide parts of it or add remove classes, depending on your application state. This is not a general guideline, because it only makes sense for small-medium apps, for larger, more complex, you probably will use some framework anyway (angular or such). Other than that, fine simple app (assuming it works, didn't run it to be honest) ;).
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.