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 Answer 1

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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.