5
\$\begingroup\$

I built a small "todo" app with an object oriented JavaScript approach.

I have spent this morning reading on a few ways to approach this, and I've come up with the following:

document.addEventListener("DOMContentLoaded", function () {
	var todo = Object.create(null);

	todo.newTodo = function (evt) {
		evt.preventDefault();
		var newTodo = document.getElementById("todo-item").value;
		var ul = document.getElementById("todo-view");
		var li = document.createElement("li");
		li.innerHTML = newTodo + '<span>x</span>';

		//using tertnary operator to add items to top of list if other items already exist
		(ul.childElementCount == 0) ? ul.appendChild(li) : ul.insertBefore(li, ul.firstChild);

		document.getElementById('todo-item').	value = "";

		var span = li.getElementsByTagName("span");

		span[0].addEventListener("click", todo.deleteTodo, false);
	}

	var todoCache = [];

	todo.deleteTodo = function () {
		todoCache.push(this.parentNode); 

		this.parentNode.parentNode.removeChild(this.parentNode); 

		return todoCache;
	}

	todo.undoDelete = function () {
		var ul = document.getElementById("todo-view");

		if (todoCache.length > 0 ) {
			var lastTodo = todoCache.length - 1;

			ul.appendChild(todoCache[lastTodo]);

			todoCache.pop();
		}
	}

	var todoSub = document.getElementById("submit-item");

	todoSub.addEventListener("click", todo.newTodo, false);

	var undo = document.getElementById('undo'); 

	undo.addEventListener("click", todo.undoDelete, false);
});
	body, html {
		width: 100%;
		height: 100%;
		margin: 0;
		padding: 0;
	}

	body {
		background: #ccc;
		font-family: 'Open Sans Condensed', sans-serif;
		font-weight: 100;
	}

	.list-container {
		padding: 15px 10px;
		margin: 0 auto;
		max-width: 30em;
		text-align: center;
	}

	.input {
		width: 100%;
	}

	.input input {
		height: 50px;
		display: block;
		width: 100%;
		margin: 0 auto;
		box-sizing: border-box;
		transition: linear .2s;
	}

	input[type="text"] {
		padding: 10px 20px;
		margin: 10px auto;
		border: none;

	}

	input[type="submit"] {
		background: #7FFFD4;
		border: none;
		color: white;
		padding: 0;
		display: block;
		cursor: pointer;
		font-size: 15px;
		text-transform: uppercase;
	}

	input[type="submit"]:hover {
		background-color: #66ccaa;
	}

	h1 {
		text-transform: uppercase;
		font-size: 40px;
		color: #fff;
	}

	ul {
		margin: 0;
		padding: 0;
	}

	li {
		list-style-type: none;
		padding: 7px 0;
		width: 100%;
		transition: .2s linear;
		position: relative;
	}

	li:hover span {
		display: inline-block;
	}

	li span {
		position: absolute;
		right: 10px;
		color: red;
		margin-right: 5px;
		display: none;
		cursor: pointer;
		font-size: 20px;
	}

	a {
		color: white;
	}
<html>
<head>
	<meta charset="UTF-8">
</head>
<link href='https://fonts.googleapis.com/css?family=Open+Sans+Condensed:300' rel='stylesheet' type='text/css'>

<body>
<div class="list-container">
	<h1>TODO List</h1>
	<a id="undo" href="javascript:;">UNDO</a>
	<ul id="todo-view">
		
	</ul>

	<div class="input">
		<form>
			<input id="todo-item" type="text" placeholder="Enter your tasks here...">
			<input id="submit-item" type="submit" value="add">
		</form>
	</div>
</div>
</body>
</html>

What do you suggest i do to improve this? Is there anything else you recommend to help further my learning efforts?

\$\endgroup\$
  • \$\begingroup\$ In span[0].addEventListener("click", todo.deleteTodo, false); the todo.deleteTodo will break if deleteTodo uses this. \$\endgroup\$ – gcampbell Jun 28 '16 at 17:31
  • \$\begingroup\$ @gcampbell are you saying 'this.deleteTodo' will break? I more than likely won't have to use that, as there's only one instance of the object, and no prototypal usage, but how i would you suggest i clean that up for future reference? \$\endgroup\$ – finesse.png Jun 28 '16 at 18:25
  • \$\begingroup\$ You could do span[0].addEventListener("click", function () { todo.deleteTodo() }). \$\endgroup\$ – gcampbell Jun 28 '16 at 18:27
  • \$\begingroup\$ Of course, it really depends on how your code changes if you add more to it, so just keep in mind JavaScript's weird this rules. \$\endgroup\$ – gcampbell Jun 28 '16 at 18:29
  • \$\begingroup\$ @gcampbell you are most certainly correct. Thank you! \$\endgroup\$ – finesse.png Jun 29 '16 at 19:30
4
\$\begingroup\$

First, your markup. From first look, it looks like a loose set of elements. I cannot tell which parts are which immediately. One method you can use is BEM. It's primarily a CSS naming convention, but can easily be used to identify which elements belong to a group.

<div class="todo">
  <h1 class="todo__header>TODO List</h1>
  <a class="todo__undo" href="#">UNDO</a>
  <ul class="todo__list">
    <li class="todo__list-item">something</li>
  </ul>
  <form class="todo__form>
    <input class="todo__input" type="text" placeholder="Enter your tasks here...">
    <input class="todo__submit" type="submit" value="add">
  </form>
</div>

In this case, you have a todo block. Inside this todo block you have todo__element. If these elements have alternate states or styles from their default, you have todo__element--alternate. Simple naming convention.

This leads us to the next issue, CSS. First, the way you write CSS is very generic. You're styling using element selectors. This is fine if the app is isolated, but if you do this inside a bigger app, you risk affecting other elements not related to the todo list.

.todo{...}
.todo__header{...}
.todo__undo{...}
/* and so on */

In this case, in conjunction with the BEM naming convention, you can easily target elements with class names. They all have the same specificity since they're all classes, and you can easily identify which groups of styles belong together and which groups of elements they affect.

Before we head over to JS, I notice that you have <a id="undo" href="javascript:;">UNDO</a>. Instead of putting a blank inline script on href, I suggest you put # and use event.preventDefault() in the click handler to prevent it from navigating. This is so that all your JS live in JS, not mixed anywhere.

Now in JS, I notice that you don't keep track of state in the script. You submit a todo, directly append to the DOM, and that's it. JS knows nothing about the todo items afterwards and state is held in the DOM. This works in small scale, but this becomes tricky when the app scales, when you do more data manipulation like item counts, multiple undos and that sort of stuff.

Following MVC and similar variations, data should live in the data layer, or just some sort of storage like a variable. View is the UI, a visual representation of the data in the model. Controller accepts interaction, usually the place where handlers and logic are written. You don't need overly complex code, but it's nice to have them clearly separated.

(function(){

  var todoList   = document.querySelector('.todo__list');
  var todoSubmit = document.querySelector('.todo__submit');
  var todoInput  = document.querySelector('.todo__input');
  var todoUndo   = document.querySelector('.todo__undo');

  // Our "model", which is just some data storage 
  var todos = [];

  // Our "controller", where all the logic goes
  function createTodo(item){
    // Return a todo object
  }

  function removeTodo(index){
    // Remove todo from todos at index
  }

  function addTodo(todo){
    // Add todo to todos
  }

  function renderTodos(){
    // For each item in todos, render an <li>
    // Add or remove from the DOM accordingly.
  }

  todoSubmit.addEventListener('click', function(){
    // get input value
    // createTodo
    // addTodo
    // clear input
    // renderTodos
  });

  todoUndo.addEventListener('click', function(){
    // get last todo
    // removeTodo
    // renderTodos
  });

}());

In the really abstract sample above, you can see that todos is the source of truth, the "model". renderTodos simply renders that array. That plus your initial DOM constitutes your "view", the UI. The controller is the logic, from the handlers to the utility functions. The only time the JS has to know about the DOM is in renderTodos. Everything else is just JS, arrays and objects. You can easily determine the number of items by getting the array length and adjust your render function accordingly.

Now you may wonder, the delete button is dynamically generated with the todo, but how would you bind if renderTodo should only construct DOM? How would the controller bind to it? That's where event delegation comes in. Events "bubble" up the DOM. Click a descendant element, the ancestors also count as clicked. We can attach an event on a parent item to listen for future elements.

// Attach event handler to todoList (the ul)
todoList.addEventListener('click', function(event){
  // check if event.target is the delete button from an li
});
\$\endgroup\$
  • \$\begingroup\$ Beautiful! Looking forward to implementing these changes and getting a better understanding as far as structure goes. From an OOP viewpoint, when i am generating new todos, should they be an instance of the initial class? or does it work to have them generated via the initial class as i did above? Hoping that makes sense. More than willing to disambiguate should it be needed. Thanks again! \$\endgroup\$ – finesse.png Jul 20 '16 at 13:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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