2
\$\begingroup\$

I'm new to programming and in front-end. It's really hard for me to learn good practices.

Right now I'm trying to learn javascript basics and bootstrap in addition. I did mine first ToDo list with bootstrap (just to train it a bit) and I feel like my JS code and HTML are just bad and should do it much better.

Can you take a look and tell me what should I do better? It works but I think not as well as it should.

const divTodo = document.querySelector('#tasks');
const addBtn = document.querySelector('#addTask');
const addInput = document.querySelector('#addTaskText');

const taskList = []

taskList.push({
    task: "Make a dinner.",
    isDone: true,
});

// Funkcje 

//Dodaj Zadanie
const addTask = function (task) {
    taskList.push({
        task: task,
        isDone: false,
    })
}

//Zmien status isDone

const changeStatus = function (index) {
    taskList[index].isDone = !taskList[index].isDone;
}

const render = function () {
    divTodo.textContent = "";

    taskList.forEach((el, index) => {

        const newRow = document.createElement('div');
        const newCheckboxDiv = document.createElement('div');
        const newCheckbox = document.createElement('input');
        const newTask = document.createElement('div');
        const newButtonDiv = document.createElement('div');
        const newButton = document.createElement('button');

        //row bg-warning m-2 d-flex align-items-center rounded
        newRow.classList.add('row')
        newRow.classList.add('m-2')
        newRow.classList.add('d-flex')
        newRow.classList.add('align-items-center')
        newRow.classList.add('rounded')
        el.isDone == true ? newRow.classList.add('bg-success') : newRow.classList.add('bg-warning');

        //col-2 col-md-1
        newCheckboxDiv.classList.add('col-2')
        newCheckboxDiv.classList.add('col-md-1')

        //form-control form-control-lg
        newCheckbox.classList.add('form-control');
        newCheckbox.classList.add('form-control-lg');
        newCheckbox.type = 'checkbox';
        el.isDone == true ? newCheckbox.checked = true : newCheckbox.checked = false;
        newCheckbox.dataset.key = `${index}`;

        newCheckbox.addEventListener('click', () => {
            el.isDone = !el.isDone;
            render();
        })

        //col-7 col-md-8 text-justify
        newTask.classList.add('col-7');
        newTask.classList.add('col-md-8');
        newTask.classList.add('text-justify');
        newTask.textContent = el.task;

        //col-3
        newButtonDiv.classList.add('col-3');

        //btn btn-danger btn-block p-1 p-md-2
        newButton.classList.add('btn');
        newButton.classList.add('btn-danger');
        newButton.classList.add('btn-block');
        newButton.classList.add('p-1');
        newButton.classList.add('p-md-2');
        newButton.textContent = "Delete";
        // newButton.dataset.key = `${index}`;
        newButton.addEventListener('click', () => {
            taskList.splice(index, 1);
            render();
        })

        newCheckboxDiv.appendChild(newCheckbox);
        newButtonDiv.appendChild(newButton);

        newRow.appendChild(newCheckboxDiv);
        newRow.appendChild(newTask);
        newRow.appendChild(newButtonDiv);

        divTodo.appendChild(newRow);
    })
}



render();

addBtn.addEventListener('click', () => {
    if (addInput.value == "") return;
    addTask(addInput.value);
    render();
    addInput.value = "";
})
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="X-UA-Compatible" content="ie=edge">
    <title>ToDo list</title>

    <!-- Bootstrap CSS -->
    <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" integrity="sha384-MCw98/SFnGE8fJT3GXwEOngsV7Zt27NXFoaoApmYm81iuXoPkFOJwJ8ERdknLPMO"
        crossorigin="anonymous">
    <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.5.0/css/all.css" integrity="sha384-B4dIYHKNBt8Bc12p+WXckhzcICo0wtJAoU8YZTY5qE0Id1GSseTk6S+L3BlXeVIU"
        crossorigin="anonymous">

</head>

<body class="bg-info">
<!---
    <div class="container mt-md-5">
         Three big buttons - doesnt work 
        <div class="row">
            <div class="col-md-12  mr-auto ml-auto text-center">
                <div class="row">
                    <div class="col-12 col-md-4 mr-auto ml-auto mt-3 mt-md-1">
                        <button class="btn btn-warning btn-block">All</button>
                    </div>
                    <div class="col-12 col-md-4 mr-auto ml-auto mt-3 mt-md-1">
                        <button class="btn btn-warning btn-block">Done</button>
                    </div>
                    <div class="col-12 col-md-4 mr-auto ml-auto mt-3 mt-md-1">
                        <button class="btn btn-warning btn-block">To do</button>
                    </div>
                </div>
            </div>

        </div>
  --->
        <!------------------------------>
        <!-- Add Tasks-->
        <div class="row">
            <div class="col-12 col-md-8 mt-2">
                <input type="text" class="form-control" placeholder="Add task..." id="addTaskText">
            </div>
            <div class="col-12 col-md-4 mt-2">
                <button class="btn btn-success btn-block" id="addTask">Add Task!</button>
            </div>
        </div>
        <!------------------------------>

        <div id="tasks">

        </div>


        <!-- Optional JavaScript -->
        <!-- jQuery first, then Popper.js, then Bootstrap JS -->
        <script src="https://code.jquery.com/jquery-3.3.1.slim.min.js" integrity="sha384-q8i/X+965DzO0rT7abK41JStQIAqVgRVzpbzo5smXKp4YfRvH+8abtTE1Pi6jizo"
            crossorigin="anonymous"></script>
        <script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.3/umd/popper.min.js" integrity="sha384-ZMP7rVo3mIykV+2+9J3UJ46jBk0WLaUAdn689aCwoqbBJiSnjAK/l8WvCWPIPm49"
            crossorigin="anonymous"></script>
        <script src="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/js/bootstrap.min.js" integrity="sha384-ChfqqxuZUCnJSK3+MXmPNIyE6ZbWh2IMqE241rYiqJxyMiZ6OW/JmZQ5stwEULTy"
            crossorigin="anonymous"></script>

        <script src="main.js"></script>

</body>

</html>

\$\endgroup\$
  • \$\begingroup\$ Looks pretty good actually! Creating DOM elements and adding multiple classes can get a bit verbose, so you might want to write a separate function for that, or you could add multiple classes by using newButton.className = "btn btn-danger btn-block p-1 p-md-2 Delete"; \$\endgroup\$ – Kokodoko Dec 13 '18 at 11:28
1
\$\begingroup\$

Many of the things done are for showing alternatives. There are many ways to do things, each with pros and cons. I have also removed the parts that were not necessary so that there are less distractions.

Used a <div> with style='display: none' to provide a template of a todo task that I clone when adding a new task.

Instead of redoing the whole list each time you do anything, it now handles the changes that need to be made as they are needed.

Event delegation used so that the whole thing only needs 2 total event handlers as opposed to 1 + 2 times number of tasks.

I added const done and todo for clarifying the classes used to indicate those states.

Hopefully this is helpful in your learning journey, good luck.

const divTodo = document.getElementById('tasks');
const addBtn = document.getElementById('addTask');
const addInput = document.getElementById('addTaskText');
const taskTemplate = document.getElementById('taskTemplate').children[0];
const done = 'bg-success';
const todo = 'bg-warning';
let newIndex = 0;

const taskList = {};

// event delegation
divTodo.addEventListener('click', function(e) {
  const target = e.target;
  const row = target.parentNode.parentNode;
  if (target.type === 'checkbox') {
    const task = taskList[target.dataset.key];
    task.isDone = !task.isDone;
    target.checked = task.isDone;
    row.classList.toggle(done);
    row.classList.toggle(todo);
  } else if (target.type === 'submit') {
    delete taskList[target.dataset.key];
    divTodo.removeChild(row);
  }
});

const addTask = function(task, initial) {
  const newTask = {
    task: task,
    isDone: initial || false,
  };

  addElement(newTask);
  taskList[newIndex] = newTask;
  newIndex++;
};

const addElement = function(el) {
  const index = newIndex;
  const newRow = taskTemplate.cloneNode(true);
  const newCheckbox = newRow.querySelector('input[type=checkbox]');
  const newTask = newRow.children[1];
  const newButton = newRow.querySelector('button');

  newRow.classList.add(el.isDone == true ? done : todo);

  newCheckbox.checked = el.isDone;
  newCheckbox.dataset.key = `${index}`;

  newTask.textContent = el.task;

  newButton.dataset.key = `${index}`;

  divTodo.appendChild(newRow);
};

addTask("Make a dinner.", true);

addBtn.addEventListener('click', () => {
  if (addInput.value == "")
    return;
  addTask(addInput.value);
  addInput.value = "";
});
<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <title>ToDo list</title>

  <!-- Bootstrap CSS -->
  <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" integrity="sha384-MCw98/SFnGE8fJT3GXwEOngsV7Zt27NXFoaoApmYm81iuXoPkFOJwJ8ERdknLPMO" crossorigin="anonymous">
  <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.5.0/css/all.css" integrity="sha384-B4dIYHKNBt8Bc12p+WXckhzcICo0wtJAoU8YZTY5qE0Id1GSseTk6S+L3BlXeVIU" crossorigin="anonymous">

</head>

<body class="bg-info">

  <div class="row">
    <div class="col-12 col-md-8 mt-2">
      <input type="text" class="form-control" placeholder="Add task..." id="addTaskText">
    </div>
    <div class="col-12 col-md-4 mt-2">
      <button class="btn btn-success btn-block" id="addTask">Add Task!</button>
    </div>
  </div>

  <div id="tasks"></div>

  <div id="taskTemplate" style="display : none">
    <div class="row m-2 d-flex align-items-center rounded">
      <div class="col-2 col-md-1">
        <input class="form-control form-control-lg" type="checkbox">
      </div>
      <div class="col-7 col-md-8 text-justify"></div>
      <div class="col-3">
        <button class="btn btn-danger btn-block p-1 p-md-2">Delete</button>
      </div>
    </div>
  </div>

</body>

</html>

\$\endgroup\$
  • 1
    \$\begingroup\$ You could also consider using a <template> tag for the template HTML \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 31 '18 at 21:52

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.