Simple Vanilla JS Todo app

Question 1

I'm a Kotlin engineer looking to get into web dev. I thought it'd be fun to make a Todo app w/o help from the internet. Just looking for some critiques and feedback, as I'm new-ish to the language.

Here's the JS, HTML, CSS. Looking for criticism on all of it. No mobile support btw.

JS:

class Column {
 
 constructor(elementId, countId) {
 this.items = new Map();
 this.count = this.items.size;
 this.elementId = elementId;
 this.countId = countId;
 this.node = document.getElementById(this.elementId);
 this.countNode = document.getElementById(this.countId);
 }
 updateCount() {
 this.count = this.items.size;
 this.countNode.innerHTML = "|(" + this.count.toString() + ")";
 }
}
class Todo extends Column {
 constructor() {
 super("todo", "todoCount");
 }
 add(item) {
 item.toolbar.appendChild(item.close);
 if (item.complete) {
 item.checkbox.addEventListener("click", () => {
 closeItem(item);
 });
 item.complete = false;
 }
 this.items.set(item, item.node);
 this.node.appendChild(item.node);
 this.updateCount()
 }
 remove(item) {
 this.items.delete(item);
 item.node.remove();
 item.complete = true;
 this.updateCount();
 }
}
class Done extends Column {
 constructor() {
 super("done", "doneCount");
 }
 add(item) {
 item.checkbox.addEventListener("click", () => {
 openItem(item);
 });
 this.items.set(item, item.node);
 this.node.appendChild(item.node);
 this.updateCount();
 }
 remove(item) {
 this.items.delete(item);
 item.node.remove();
 this.updateCount();
 }
}
class TodoItem {
 constructor() {
 this.node = null;
 this.toolbar = null;
 this.checkbox = null;
 this.close = null;
 this.complete = false;
 }
 generateName() {
 return Math.floor(Math.random() * 1000).toString();
 }
 create() {
 // Create the parent of the todo item
 let item = document.createElement("article");
 item.id = this.generateName();
 // Create a checkbox, and its parent
 let todoToolbar = document.createElement("span");
 todoToolbar.className = "todoToolbar";
 let checkbox = document.createElement("input");
 checkbox.className = "checkbox";
 checkbox.type = "checkbox";
 let close = document.createElement("span");
 close.className = "close"
 close.textContent = "✖";
 // Create the Todo input area
 let input = document.createElement("textarea");
 input.className = "todoInput";
 checkbox.addEventListener("click", () => {
 closeItem(this);
 });
 close.addEventListener("click", () => {
 deleteItem(this);
 });
 // Assign children and set the node
 todoToolbar.appendChild(checkbox);
 todoToolbar.appendChild(close);
 item.appendChild(todoToolbar);
 item.appendChild(input);
 this.node = item;
 this.toolbar = todoToolbar;
 this.checkbox = checkbox;
 this.close = close;
 }
}
let todo = new Todo();
let done = new Done();
function closeItem(item) {
 todo.remove(item);
 done.add(item);
}
function openItem(item) {
 done.remove(item);
 todo.add(item);
}
function deleteItem(item) {
 if (done.items.has(item)) {
 done.items.delete(item);
 done.updateCount();
 }
 if (todo.items.has(item)) {
 todo.items.delete(item);
 todo.updateCount();
 }
 item.node.remove();
}
function createItem() {
 let item = new TodoItem();
 item.create();
 todo.add(item);
}
function main() {
 document.getElementById("addTodo").onclick = createItem;
 todo.updateCount();
 done.updateCount();
}
main();

HTML:

<!DOCTYPE html>
<html lang="en">
<head>
 <meta charset="UTF-8">
 <link rel="stylesheet" href="index.css">
 <title>Title</title>
</head>
<body>
 <main id="todoParent">
 <article id="functionality">
 <section id="toolbar">
 <button id="addTodo">Add Todo</button>
 </section>
 <section id="columns">
 <section id="todo">
 <div class="columnToolbar">
 <h1>Todo</h1>
 <h1 id="todoCount"></h1>
 </div>
 </section>
 <section id="done">
 <div class="columnToolbar">
 <h1>Done</h1>
 <h1 id="doneCount"></h1>
 </div>
 </section>
 </section>
 </article>
 </main>
 <script src="index.js"></script>
</body>
</html>

CSS:

html {
 min-height: 100%;
 position: relative;
}
body {
 height: 100%;
}
button {
 z-index: 10;
}
#todoParent {
 display: flex;
 justify-content: center;
 height: 100%;
 top:0;
 bottom:0;
 left:0;
 right:0;
 overflow:hidden;
 background-color: darkslategray;
 position:absolute;
}
#functionality {
 width: 40%;
 background-color: white;
}
#columns {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
#columns h1 {
 text-align: center;
}
#columns section {
 background-color: #e0e0e0;
 border: black solid 1px;
 width: 250px;
 height: 90vh;
 margin: 5%;
 overflow-y: scroll;
}
#columns article {
 display: flex;
 flex-direction: column;
 justify-content: flex-start;
 background-color: white;
 width: 200px;
 height: 200px;
 margin: 5%;
 padding: 5%;
}
#toolbar {
 display: flex;
 justify-content: center;
 flex-direction: row;
 background-color: #e0e0e0;
}
#toolbar button {
 margin: 5px;
}
#todoCount {
 margin-left: 1%;
}
.todoToolbar {
 display: flex;
 flex-direction: row;
 justify-content: space-between;
}
.todoCheckbox {
 margin: 5%;
}
.todoInput {
 height: 100%;
 box-sizing: border-box;
 resize: vertical;
}
.columnToolbar {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
```

Question 2

Too complex

You have over engineered the objects.

Column should not care what it contains. All it should do is add and remove items.

Inheriting from Column to create Done and Todo columns has just made more work for the coder.

Only the Todo item cares about where it is. It adds it's self to the Todo column if not complete, moves it's self from the todo to done when UI clicked. and removes it's self when deleted.

There should be no calls to outside function to provide this functionality.

Rewrite

Not addressing various other items the rewrite is an example of a more role driven design. This example does not include any state encapsulation as the focus is on defining clear object roles rather than state safty.

Object `Column`

new Column(node, countNode); Accepts nodes rather than ID's
Column.add(item) adds an item if it has a node to add. It also calls the items added function if item has that function,
Column.remove(item) Removes the item if it has it.

Object `Todo`

new Todo() Creates nodes and adds listeners and adds its self to the todoColumn
Todo.added(column) Called by instance of Column to provide a reference to the column
Todo.toggleComplete() Event. Toggles the complete semaphore. It removes and adds it's self to the correct column as required.
Todo.delete() Event. Removes it's self from current column,

Note that the functions tag, append, listener are just helper functions to reduce the verbosity of DOM API calls.

;(()=>{
"use strict";
const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);
const listener = (el, name, call, opt = {}) => (el.addEventListener(name, call, opt), el);
var uid = 0;
class Column {
 constructor(node, countNode) {
 this.items = new Map();
 this.node = node;
 this.countNode = countNode;
 }
 updateCount() { this.countNode.textContent = " | (" + this.items.size + ")" }
 add(item) {
 if (item.node) {
 item?.added(this);
 this.items.set(item, item.node);
 this.node.appendChild(item.node);
 this.updateCount();
 }
 }
 remove(item) {
 if (this.items.has(item)) {
 this.items.delete(item);
 item.node.remove();
 this.updateCount();
 }
 } 
}
class Todo {
 constructor() {
 this.complete = false;
 this.node = tag("article", {id: uid++});
 const toolbar = tag("span", {className: "todoToolbar"});
 const checkbox = listener(
 tag("input", {className: "checkbox", type: "checkbox"}),
 "click", this.toggleComplete.bind(this)
 );
 const close = listener(
 tag("span", {className: "close", textContent: "✖"}),
 "click", this.delete.bind(this)
 );
 
 append (
 this.node,
 append(toolbar, checkbox, close),
 tag("textarea", {className: "todoInput"})
 );
 todoColumn.add(this);
 }
 delete() { this.column.remove(this) } 
 added(column) { this.column = column }
 toggleComplete() {
 this.column.remove(this);
 this.complete = !this.complete;
 (this.complete ? doneColumn : todoColumn).add(this); 
 }
}
const todoColumn = new Column(todo, todoCount);
const doneColumn = new Column(done, doneCount);
listener(addTodo, "click", () => new Todo());
})();

#todoParent {
 display: flex;
 justify-content: center;
 height: 100%;
 top:0;
 bottom:0;
 left:0;
 right:0;
 overflow:hidden;
 background-color: darkslategray;
 position:absolute;
}
#functionality {
 width: 84%;
 background-color: white;
}
#columns {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
#columns h1 {
 text-align: center;
}
#columns section {
 background-color: #e0e0e0;
 border: black solid 1px;
 width: 250px;
 height: 90vh;
 margin: 5%;
 overflow-y: scroll;
}
#columns article {
 display: flex;
 flex-direction: column;
 justify-content: flex-start;
 background-color: white;
 width: 82%;
 margin: 5%;
 padding: 5%;
}
#toolbar {
 display: flex;
 justify-content: center;
 flex-direction: row;
 background-color: #e0e0e0;
}
#toolbar button { margin: 5px; }
#todoCount { margin-left: 1%; }
.todoToolbar {
 display: flex;
 flex-direction: row;
 justify-content: space-between;
}
.columnToolbar {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
.close { cursor: pointer; }
.close:hover { color: red; }

<main id="todoParent">
 <article id="functionality">
 <section id="toolbar">
 <button id="addTodo">Add Todo</button>
 </section>
 <section id="columns">
 <section id="todo">
 <div class="columnToolbar">
 <h1>Todo</h1>
 <h1 id="todoCount"></h1>
 </div>
 </section>
 <section id="done">
 <div class="columnToolbar">
 <h1>Done</h1>
 <h1 id="doneCount"></h1>
 </div>
 </section>
 </section>
 </article>
</main>

Blindman67 Blindman67 22.8k2 gold badges16 silver badges40 bronze badges · Answer 1 · 2021-09-27 15:23:17Z

Too complex

You have over engineered the objects.

Column should not care what it contains. All it should do is add and remove items.

Inheriting from Column to create Done and Todo columns has just made more work for the coder.

Only the Todo item cares about where it is. It adds it's self to the Todo column if not complete, moves it's self from the todo to done when UI clicked. and removes it's self when deleted.

There should be no calls to outside function to provide this functionality.

Rewrite

Not addressing various other items the rewrite is an example of a more role driven design. This example does not include any state encapsulation as the focus is on defining clear object roles rather than state safty.

Object `Column`

new Column(node, countNode); Accepts nodes rather than ID's
Column.add(item) adds an item if it has a node to add. It also calls the items added function if item has that function,
Column.remove(item) Removes the item if it has it.

Object `Todo`

new Todo() Creates nodes and adds listeners and adds its self to the todoColumn
Todo.added(column) Called by instance of Column to provide a reference to the column
Todo.toggleComplete() Event. Toggles the complete semaphore. It removes and adds it's self to the correct column as required.
Todo.delete() Event. Removes it's self from current column,

Note that the functions tag, append, listener are just helper functions to reduce the verbosity of DOM API calls.

;(()=>{
"use strict";
const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);
const listener = (el, name, call, opt = {}) => (el.addEventListener(name, call, opt), el);
var uid = 0;
class Column {
 constructor(node, countNode) {
 this.items = new Map();
 this.node = node;
 this.countNode = countNode;
 }
 updateCount() { this.countNode.textContent = " | (" + this.items.size + ")" }
 add(item) {
 if (item.node) {
 item?.added(this);
 this.items.set(item, item.node);
 this.node.appendChild(item.node);
 this.updateCount();
 }
 }
 remove(item) {
 if (this.items.has(item)) {
 this.items.delete(item);
 item.node.remove();
 this.updateCount();
 }
 } 
}
class Todo {
 constructor() {
 this.complete = false;
 this.node = tag("article", {id: uid++});
 const toolbar = tag("span", {className: "todoToolbar"});
 const checkbox = listener(
 tag("input", {className: "checkbox", type: "checkbox"}),
 "click", this.toggleComplete.bind(this)
 );
 const close = listener(
 tag("span", {className: "close", textContent: "✖"}),
 "click", this.delete.bind(this)
 );
 
 append (
 this.node,
 append(toolbar, checkbox, close),
 tag("textarea", {className: "todoInput"})
 );
 todoColumn.add(this);
 }
 delete() { this.column.remove(this) } 
 added(column) { this.column = column }
 toggleComplete() {
 this.column.remove(this);
 this.complete = !this.complete;
 (this.complete ? doneColumn : todoColumn).add(this); 
 }
}
const todoColumn = new Column(todo, todoCount);
const doneColumn = new Column(done, doneCount);
listener(addTodo, "click", () => new Todo());
})();

#todoParent {
 display: flex;
 justify-content: center;
 height: 100%;
 top:0;
 bottom:0;
 left:0;
 right:0;
 overflow:hidden;
 background-color: darkslategray;
 position:absolute;
}
#functionality {
 width: 84%;
 background-color: white;
}
#columns {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
#columns h1 {
 text-align: center;
}
#columns section {
 background-color: #e0e0e0;
 border: black solid 1px;
 width: 250px;
 height: 90vh;
 margin: 5%;
 overflow-y: scroll;
}
#columns article {
 display: flex;
 flex-direction: column;
 justify-content: flex-start;
 background-color: white;
 width: 82%;
 margin: 5%;
 padding: 5%;
}
#toolbar {
 display: flex;
 justify-content: center;
 flex-direction: row;
 background-color: #e0e0e0;
}
#toolbar button { margin: 5px; }
#todoCount { margin-left: 1%; }
.todoToolbar {
 display: flex;
 flex-direction: row;
 justify-content: space-between;
}
.columnToolbar {
 display: flex;
 flex-direction: row;
 justify-content: center;
}
.close { cursor: pointer; }
.close:hover { color: red; }

<main id="todoParent">
 <article id="functionality">
 <section id="toolbar">
 <button id="addTodo">Add Todo</button>
 </section>
 <section id="columns">
 <section id="todo">
 <div class="columnToolbar">
 <h1>Todo</h1>
 <h1 id="todoCount"></h1>
 </div>
 </section>
 <section id="done">
 <div class="columnToolbar">
 <h1>Done</h1>
 <h1 id="doneCount"></h1>
 </div>
 </section>
 </section>
 </article>
</main>

Stack Exchange Network

Simple Vanilla JS Todo app

1 Answer 1

Too complex

Rewrite

Object `Column`

Object `Todo`

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Simple Vanilla JS Todo app

1 Answer 1

Too complex

Rewrite

Object Column

Object Todo

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions

Object `Column`

Object `Todo`