6
\$\begingroup\$

I'm making a simple todo list app with ES6. I'm looking for some feedback on the code, I feel like there is some improvement possible. Maybe there is a better way to attach eventListeners to input and button, but to keep in mind they have to be set not just when a new element is added but on window load when the items from localStorage are retrieved. I tried to keep it short as best I could.

import TaskItem from './TaskItem'; //constructor for id, title, and completed boolean
import * as helpers from './Helpers'; //two extra methods

export default class TaskList {

    constructor()
    {
        this.inputField= document.getElementById('task-input');
        this.addButton = document.getElementById('add-task');
        this.tasksElem = document.getElementById('tasks');
        this.tasks = JSON.parse(localStorage.getItem('userTasks')) || [];

        this.displayTasks()
        this.initializeEventListeners()

    }

    initializeEventListeners()
    {
        this.tasksElem.addEventListener('click', (event) => {
            const taskId = event.target.parentNode.getAttribute('data-id');

            switch (event.target.localName) {
                case 'button':
                    this.removeTask(taskId);
                    break;
                case 'input':
                    this.toggleCompleted(taskId);
                    break;
                default:
            }
        })


        this.addButton.addEventListener('click', this.addNewTask.bind(this))

    }

    addNewTask()
    {
        const newTaskTitle = this.inputField.value.trim();

        if (newTaskTitle)
        {
            let newTaskID = helpers.getRandomId().toString(); //still need to check if id exists
            const newTask = new TaskItem(newTaskID, helpers.htmlEscape(newTaskTitle))
            this.tasks.push(newTask)
            this.inputField.value = '';
            this.saveTasks()
            this.displayTasks()
        }

    }
    
    removeTask(idToRemove)
    {
        this.tasks = this.tasks.filter((task) => {
            return task.id !== idToRemove
        });

        this.saveTasks()
        this.displayTasks()
    }
    toggleCompleted(idToToggle)
    {
        this.tasks.forEach((task)=>{
            if (task.id === idToToggle) {
                task.completed = !task.completed;
            }
        })

        this.saveTasks()
        this.displayTasks()
    }

    saveTasks()
    {
              localStorage.setItem('userTasks',JSON.stringify(this.tasks));
    }


    displayTasks()
    {
        this.tasksElem.innerHTML = this.tasks.reduce((acc, task) => {
            return acc + this.returnTaskTemplate(task)
        },'');

    }


    returnTaskTemplate(task)
    {
        return `<li data-id="${task.id}">
                   <input class="toggle" type="checkbox" ${task.completed ? 'checked' : ''}>
                   <label >${task.title}</label>
                   <button>Remove</button>
                </li>`;
    }
}

Any feedback is appreciated.

\$\endgroup\$
1
\$\begingroup\$

I would prefer a better extensibility (for example, easy to add new actions) and to get the taskId only if I’m going to use it.

initializeEventListeners() {
    const actions = {
        button: 'removeTask',
        input: 'toggleCompleted'
    };

    this.tasksElem.addEventListener('click', (event) => {
        const action = actions[event.target.localName];
        if (action) {
            const taskId = event.target.parentNode.getAttribute('data-id');
            this[action](taskId);
        }
    })

    this.addButton.addEventListener('click', this.addNewTask.bind(this))
}
\$\endgroup\$

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.