Created a todo list in TS

Question

I am learning Typescript and created a straightforward to-do list project to practice some basic concepts.

I am able to

Edit the todo
Delete the todo
Mark the todo as completed

I want to how can I better structure my code and what typescript best practices I am not using.

index.ts

import { v4 as uuidv4 } from 'uuid';

const ul = document.querySelector("#todo")as HTMLUListElement
const addTaskBtn = document.querySelector('#add-btn') as HTMLButtonElement
const input = document.querySelector("#input") as HTMLInputElement

//  array which stores the object 
let todos:Array<Todo> = []
let id: string;

interface Todo {
    id: string;
    todo: string; 
    completed: boolean;
    createdDate: Date;
    updatedDate?: Date;
} 

// How do I create objects that are  stored on the array?
type storeObject = (a: string) => void;
const storeObject = (todo: string)=>{
    const item: Todo = {
        id: uuidv4(),
        todo: todo,
        completed: false ,
        createdDate: new Date,
    }
    todos.push(item)
}

const renderTodos = ()=> {
    todos.forEach((obj)=>{      
        const div = document.createElement("div");
        div.classList.add("item")
        
        const li = document.createElement("li");
        li.classList.add("todo")

        const checkbox = document.createElement("input")
        checkbox.type = "checkbox"
        checkbox.id = `${obj.id}`
        if(obj.completed) checkbox.checked = true

        const label = document.createElement("label") 
        label.textContent = obj.todo
        label.htmlFor = `${obj.id}`

        const btns = document.createElement("div");
        
        const editBtn = document.createElement("button") as HTMLButtonElement
        editBtn.textContent = "Edit "
        editBtn.classList.add("editBtn")
        
        const deleteBtn = document.createElement("button") as HTMLButtonElement
        deleteBtn.textContent = "Delete"
        deleteBtn.classList.add("deleteBtn")

        btns.appendChild(editBtn)
        btns.appendChild(deleteBtn)

        li.appendChild(checkbox)
        li.appendChild(label)

        div.appendChild(li);
        div.appendChild(btns);

        ul.appendChild(div);
    })
}

const updateTodo = (id:string)=>{
    //  Updating todo after finding todo using id 
    todos.map((item)=>{
        if(item.id == id) item.todo = input.value;
        return 
    })
}

addTaskBtn.addEventListener("click",(e)=>{
    if(input.value == "") return 
    
    (e.target as HTMLElement).textContent == "Update" ?
        updateTodo(id) :
        storeObject(input.value);
    
    input.value = ""
    // dom should be cleared and painted according to new data in array 
    ul.innerHTML = ""
    addTaskBtn.textContent = "Add Task" 

    renderTodos()
});

document.addEventListener("click", (e)=>{
    // getting the clicked todo's id  
    if((e.target as Element).classList.contains("editBtn")){
        id = ((e.target as HTMLElement).parentNode.parentNode.childNodes[0].childNodes[0] as HTMLElement).id
        let todo:string = ((e.target as HTMLElement).parentNode.parentNode.childNodes[0].childNodes[1] as HTMLElement).innerText

        // populate input with the value 
        input.value = todo;
        addTaskBtn.textContent = "Update"
    }else if((e.target as HTMLElement).classList.contains("deleteBtn")){
        // deleting todo 
        id = ((e.target as HTMLElement).parentNode.parentNode.childNodes[0].childNodes[0] as HTMLElement).id

        let values = todos.filter((item)=>{
            if(item.id !== id) return item
        })
        todos = []
        todos = [...values]
        ul.innerHTML = ''
        renderTodos()
    }else if (((e.target as HTMLElement).parentNode as HTMLElement).classList.contains("todo")){
        // checking if checkbox clicked
        id = ((e.target as HTMLElement).parentNode.parentNode.childNodes[0].childNodes[0] as HTMLElement).id;
        let values;
        if(((e.target as HTMLElement).parentNode.childNodes[0] as HTMLInputElement).checked) {
            values = todos.map((item)=>{
                if(item.id == id) {
                    item.completed = true
                    return item 
                }
                return item 
            })
        }else{
            values = todos.map((item)=>{
                if(item.id == id) {
                    item.completed = false
                    return item 
                }
                return item 
            })
        }
        todos = []
        todos = [...values]
        ul.innerHTML = ""
        renderTodos()
    }
})

index.html

    <main>
        <div class="top">
            <input type="text" name="todo-input" id="input" value="" placeholder="TODO" >
            <button id="add-btn" class="add-btn">Add Task</button>
        </div>
        <ul id="todo">
        </ul>
    </main>

Working example of todolist: https://codesandbox.io/s/condescending-newton-b7ljoz?file=/src/index.ts

Dave Meehan · Accepted Answer · 2022-06-28 20:55:37Z

storeObject cannot be both a type and a function. Remove the type storeObject... statement
new Date needs parentheses, e.g. new Date()

storeObject does not need the variable item, the object can be inlined, e.g.

todos.push({
    id: uuidv4(),
    todo,
    completed: false,
    createdDate: new Date(),
})

Don't use type assertions (e.g. ... as SomeType), use type guards and narrowing. For example, In addTaskBtn event listener, use:

if (e.target instanceof HTMLElement) {
    if (e.target.textContent === "Update") {
        updateTodo(id)
    } else {
        storeObject(input.value);
    }
}

Alternatively, check at the start of the method that you have the prerequsites and bail as appropriate:

if(input.value === "" || !(e.target instanceof HTMLElement)) {
    return 
}

if (e.target.textContent === "Update") {
    updateTodo(id)
} else {
    storeObject(input.value);
}

if (e.target.textContent === "Update") {
    updateTodo(id)
} else {
    storeObject(input.value);
}

Use strict equality === instead of == or type coercion can lead to unexpected results.
Don't use ternary expressions with side effects, use a full if/then/else expression.
Use optional chaining operator (?.) to avoid needing to check for null, or type assertions to override the possibility of null. If your DOM does not match the expected, you should probably bail in the event that DOM queries return NULL. Alternatively, put the DOM setup into the code, and only require a container element to be passed to the setup method.
updateTodo: todos.map's callback should return a value (e.g. return item)
You're creating a lot of work for yourself to add an event listener to the document that then has to disambiguate which element is firing the event. Instead, add event handlers to the specific element they serve.

Instead of inlining the event handler, take advantage of the fact that this is the event target (you'll need to use a fat function rather than an arrow function). e.g.

function editBtnHandler(this: HTMLButtonElement, ev: Event) {
   // this === ev.target
   // it's also correctly typed, so we don't need type assertions or null checks
}

editBtn.addEventListener('click', editBtnHandler)

Wrap the whole shebang in a class. Pass to the constructor either a container element, or validated list, button and input elements so that you can lose the type assertions and optional chaining.

class TodoApp {
    private todos: Todo[] = []

    constructor(container: HTMLElement) {
        const ul = document.createElement(...)
        //...
        container.appendChild(ul)
        //...
    }

    renderTodos() {
        this.todos.forEach(todo => {
            //...
        })
    }
}
const container = document.querySelector('#todoApp')
const todoApp = new TodoApp(container)

Reconsider how you do the DOM traversal to get to elements containing id and label text. If you are adding a dedicated handler you could bind those values when the handler is created, so you have them to hand when handling the event.
Rather than redefining the functionality of the button/input element depending on add/update, you could use CSS classes to show/hide elements dedicated to those tasks (or update the DOM to add/remove). This way you lower the risk of keeping track of which state any element is in, and the code will read more clearly.

There is a lot there for you to do, and rather than just providing a rewrite with all of those suggestions, its probably better that you work through them and see how they apply. Come back when you have a revision, perhaps as a new post. Feel free to ask for clarifications if not sure.

Thank you so much for all these suggestions. I don't understand a few things but I will try to figure them out. If I am stuck I will ask again — Mubashir Waheed
– Mubashir Waheed, Commented Jun 28, 2022 at 17:55
you said in the 9th point "...add event handlers to the specific element they serve.". How this can be done? I am asking because the todos are created dynamically and how should I add distinctive ids to each button and then add listeners for each of them Also, can you explain the 10th and 11th points? any code example to help me understand would be great — Mubashir Waheed
– Mubashir Waheed, Commented Jun 28, 2022 at 19:08
addTaskBtn.addEventListener("click"... seems you already know how to add handlers to specific elements. When you create editBtn, deleteBtn etc in renderTodos, you can attach handlers to the elements as you create them. Extract the common logic into a function, and use a closure to capture the context needed for them to do their work (for example the array index of the todos array). This way you don't need to add attributes to the HTML elements to provide context (although that can be a valid approach too). — Dave Meehan
– Dave Meehan, Commented Jun 28, 2022 at 20:43

Stack Exchange Network

Created a todo list in TS

1 Answer 1

You must log in to answer this question.

Linked

Hot Network Questions

Created a todo list in TS

1 Answer 1

You must log in to answer this question.

Linked

Related

Hot Network Questions