Task-list app, which allows to add and remove tasks from list

Question

I wrote my first web app. Please let me know what I should improve. I want to write as clean code as possible.

App is about adding and removing tasks from list. Backend is written in Django, frontend in Sass and JavaScript.

Below my code for items view. Whole project can be seen in link:

Link to my code on GitHub

Demo:

Link to app on heroku

Code for items view:

1. JavaScript
2. Sass
3. HTML

// Start with first item
let counter = 0;

// Load 6 items at a time
const QUANTITY_START = 6; 
const QUANTITY_TO_ADD_ON_SCROLL = 3; 

let isListEmpty = true;
let removed = [];
let areMoreItems = true;

const HIDE_ANIMATION_DURATION_OPACITY = 1500

document.addEventListener('DOMContentLoaded', () => {

  // SETTING LISTENERS:
  window.onscroll = () => {
    if (areMoreItems && isScrolledToBottom()){
      const scroll = window.onscroll;
      window.onscroll = "none";
      load(QUANTITY_TO_ADD_ON_SCROLL);
      setTimeout(() => {
        window.onscroll = scroll;
      }, 1000);
    } 
  }

  deleteItemListener();
  confirmDeleteButtonListener();
  // END OF SETTING LISTENERS

  load(QUANTITY_START);
})

function confirmDeleteButtonListener() {
  const confirm_button = document.querySelector(".confirm-delete-button");
  confirm_button.addEventListener("click", () => {
    confirm_button.value = removed.join();
  })
}

function deleteItemListener() {
  document.querySelector(".main").addEventListener("click", function(event) {
    if(event.target.nodeName === "I") {

      const itemElement = event.target.parentElement.parentElement;
      const item_id = itemElement.querySelector('.id').innerHTML;
      removed.push(item_id);

      itemElement.style.animationPlayState = 'running';
      // itemElement.addEventList;

      document.querySelector(".confirm-delete-button").disabled = false;
    }
  });
}

function isScrolledToBottom() {
  return window.innerHeight + window.scrollY >= document.body.offsetHeight
}

function load(quantity) {

  const start = counter;
  const end = start + quantity - 1;
  counter = end + 1;

  fetch(`itemslist?start=${start}&end=${end}`)
  .then(response => response.json())
  .then(data => {
    if (data.itemslist.length > 0) {
      isListEmpty = false;
      data.itemslist.forEach(addItem);
    } else {
      areMoreItems = false;
      if (isListEmpty) {
        addEmptyListInfo();
      }
    }
  });
}

function addEmptyListInfo() {
  const item = document.createElement('section');
  item.className = "empty-list";
  item.innerText = "Your list is empty";
  document.querySelector('.main-items').append(item)
}

function addItem(item) {
  const itemNode = document.createElement('section');
  itemNode.className = "todo-item";
  itemNode.innerHTML =     
    `<p hidden class="id">${item.id}</p>
    <div name="delete-button" value="${item.id}" class="x-wrapper">
      <i class="far fa-times-circle"></i>
    </div>
    <h2 name="name" class="todo-item-name">${item.name}</h2>
    <label for="details">Details:</label>
    <p class="details" name="details">${item.details}</p>
    <p class="date">Deadline: ${item.date}</p>`;
  document.querySelector('.main-items').append(itemNode);
};

.nav {
  .li {
    background-color: transparent;
    border: none;
    cursor: pointer;
    font-family: 'Caveat', cursive;
    color: #fff;
    outline: none;
  }

  form {
    display: flex;
    display: -webkit-box;
    display: -ms-flexbox;
    flex-direction: row;
  }
}

.main-items {
  opacity: 0.9;
  display: -webkit-box;
  display: -ms-flexbox;
  display: flex;
  flex-direction: row !important;
  justify-content: space-evenly;
  flex-wrap: wrap;

  .todo-item {
    display: -webkit-box;
    display: -ms-flexbox;
    display: flex;
    -webkit-box-orient: vertical;
    -webkit-box-direction: normal;
        -ms-flex-direction: column;
            flex-direction: column;
    color: #000;
    height: 300px;
    width: 400px;
    padding: 20px;
    margin: 0 15px;
    position: relative;
    -webkit-animation-name: hide;
            animation-name: hide;
    -webkit-animation-duration: 1.5s;
            animation-duration: 1.5s;
    -webkit-animation-fill-mode: forwards;
            animation-fill-mode: forwards;
    -webkit-animation-play-state: paused;
            animation-play-state: paused;
    background-image: url(../img/sticky1.png);
    background-size: 400px 300px;
    line-height: 100%;

    .todo-item-name {
      display: -webkit-box;
      display: -ms-flexbox;
      display: flex;
      -webkit-box-pack: center;
         -ms-flex-pack: center;
       justify-content: flex-start;
      -webkit-box-align: center;
         -ms-flex-align: center;
            align-items: center;
      margin-bottom: 10px;
      margin-left: 40px;
      margin-right: 25px;
      height: 40px;
      font-size: 25px;
      font-weight: bold;
      margin-top: 20px;
      padding: 5px;
      outline: none;
      color: #000;
      overflow-y: auto;
      scrollbar-width: thin;
      scroll-padding: 0;
      scroll-margin: 0;
      white-space: nowrap;
      cursor: default;
    }

    .details {
      border-radius: 5px;
      overflow-wrap: break-word;
      overflow-y: auto;
      height: 100px;
      padding: 10px;
      margin-left: 40px;
      margin-right: 25px;
      text-align: justify;
      resize: none;
      margin-bottom: 10px;
      outline: none;
      font-size: 22px;
      scrollbar-width: thin;
      scroll-padding: 0;
      scroll-margin: 0;
      cursor: default;
      }

    .x-wrapper {
      position: absolute;
      right: 0;
      top: 0;
      margin: 10px;
      font-size: 20px;
      -webkit-transition: all 0.2s;
      transition: all 0.2s;
      cursor: pointer;
      border: none;
      color: #e9c1c1;
      background-color: transparent;
    }

    .x-wrapper:hover {
      color: #fff;
    }
  }

  .empty-list {
    font-size: 44px;
    margin-bottom: 40px;
    background-color: rgba(0, 0, 0, 0.5);
    padding: 10px;
    padding-right: 15px;
    border-radius: 10px;
    text-align: center;
    pointer-events: none;
  }

  .todo-item label {
    font-size: 20px;
    margin-bottom: 2px;
    margin-left: 40px;
    margin-right: 25px;
  }

  .date {
    text-align: right;
    margin-right: 25px;
    pointer-events: none;
  }
}

@-webkit-keyframes hide {
  0% {
    opacity: 1;
  }
  100% {
    opacity: 0;
  }
}

@keyframes hide {
  0% {
    opacity: 1;
  }
  100% {
    opacity: 0;
  }
}

{% extends "TodoListApp/base.html" %}
{% load static %}

{% block style %}
  <link rel="stylesheet" href="{% static 'TodoListApp/css/items.css' %}">
  <link rel="stylesheet" href="{% static 'TodoListApp/css/loader.css' %}">
  <title>Items</title>
{% endblock %}

{% block body %}
  <div id="loader-wrapper">
    <div id="loader"></div>
    <div class="section-left"></div>
    <div class="section-right"></div>
  </div>

  <nav class="nav">
    <a href="{% url 'newitem' %}">
      <div class="li">Add new item</div>
    </a>
    <form action="{% url 'items' %}" method="post">
      {% csrf_token %}
      <button class="add-10-items li" value="add-10-items" name="add-10-items">add 10 items</button>
      <button disabled class="confirm-delete-button li" value="" name="confirm-delete-button">confirm deleting</button>
    </form>
    <a href="{% url 'logout' %}">
      <div class="li">Logout</div>
    </a>
  </nav>

  <div class="container container-items">
    <header class="header">
      <h1>Hello :{{ name }}:</h1>
      <h1>Here is a list of your activities:</h1>
    </header>
    <main class="main main-items">

    </main>
  </div>

  <script src="{% static 'TodoListApp/js/items.js' %}"></script>
  <script src="{% static 'TodoListApp/js/loader.js' %}"></script>
{% endblock %}

Answer 1

Adding elements

As pointed out in "CertainPerformance's" answer adding content via HTML in this case can present a security risk.

However adding elements via innerHTML should always be avoided as it is very inefficient, noisy, and hard to read and maintain (due to there being no code formatting in a string)

With a few simple functions you can avoid the very slow setter .innerHTML and add content in an efficient way, without the verbosity of the DOM API.

The following set of functions can take care of most of your HTML creation needs

• tag creates an element and adds properties, returning the new tag.
• append appends siblings to an element, returning the parent.
• query queries an element returning the found element (if found)

Code

const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);
const query = (qStr, el = document) => el.querySelector(qStr);

And would replace your code with

function load(quantity) {
    const url = `itemslist?start=${counter}&end=${counter + quantity - 1}`;
    const mainItems = query(".main-items");
    counter = end + 1;

    fetch(url)
        .then(response => response.json())
        .then(data => {
            if (data.itemslist.length > 0) {
                isListEmpty = false;
                append(mainItems, ...data.itemslist.map(addItem));
            } else {
                areMoreItems = false;
                isListEmpty && append(mainItems, addEmptyListInfo());
            }
        });
}

function addEmptyListInfo() {
  return tag("section", {className: "empty-list", textContent: "Your list is empty"});
}
function addItem(item) {
    return append(tag("section", {className: "todo-item"}),
        tag("p",     {hidden: true, className: "id", textContent: item.id}),
        append(tag("div", {name: "delete-button", value: item.id, className:"x-wrapper"}), 
            tag("i", {className: "far fa-times-circle"})
        ),
        tag("h2",    {name: "name", className: "todo-item-name", textContent: item.name}),
        tag("label", {for: "details", textContent: "Details:"}),
        tag("p",     {className: "details", name:"details", textContent: item.details}),
        tag("p",     {className: "date", textContent: `Deadline: ${item.date}`}),
    );
}

Make the code work for YOU!

Personally I use a modification combing tag and query and naming it $ (as I never use jQuery) and name append $$ with helper functions to simplify common properties. Example $.text, $.classText, $.class, create properties, textContent, className and textContent, and className plus any additional properties.

There are dozens of ways you can simplify adding content to the page. And the best thing it is lightning fast, you can really notice the speed increase when building a page using the DOM API calls

function addItem(item) {
    return $$( $("<section>", $.class("todo-item")),
        $("<p>", $.classText("id", item.id, {hidden: true})),
        $$( $("<div>", $.class("x-wrapper", {name: "delete-button", value: item.id}), 
            $("<i>", $.class("far fa-times-circle"))
        ),
        $("<h2>", $.classText("todo-item-name", item.name, {name: "name"})),
        $("<label>", $.text("Details:", {for: "details"})),
        $("<p>", $.classText("details", item.details, {name:"details"})),
        $("<p>", $.classText("date", `Deadline: ${item.date}`))
    );
}

Answer 2

Pretty good code, I'd remove variable isListEmpty as it does nothing at all, your if else branch already checks for empty list and you get rid of one ugly global variable.

Consider getting rid of all the global variables by wrapping functions, that need them into either into class or another function. that way variables are not global, and you can even pass some of these as parameters (ex: QUANTITY_START).

addEmptyListInfo and addItem have identical first and last line. You can extract middle of them into separate functions and create 3rd common function to avoid redundancy and make sure adding new "section" is only in one place.

I don't like that fetch url is hardcoded into .js file. Consider extracting it somewhere into html as data-attribute, that way you can control how is url generated and if you change django routing, you don't have to modify url in multiple places.

Consider extracting debounce listener of scroll into separate function, example: https://stackoverflow.com/a/12009497/4070660

As for scss, you can use autoprefixer to handle vendor prefixes for you and clean up your code a bit.

Most of time it's better to use other units than pixels. Consider using em/rem or vw/vh/

Nothing comes to mind when checking your html :-)

Answer 3

Use const when possible - using let is a warning to readers of the code that a variable may get reassigned in the future. If that's not actually the case, better to use const for the removed variable; consider a linter.

removed array? Its purpose isn't entirely clear at first glance, I had to read over a few sections of the code to understand what it's doing. Consider naming it itemIdsToBeRemoved. Also, counter might be better named lastItemIdLoaded or something of the sort.

Use camelCase for plain variables, as is the overwhelming convention in JS: eg, consider using itemId and confirmButton. (The all-caps numeric constants at the top are just fine.)

DOMContentLoaded section You do 4 things there:

• Add a scroll listener
• Call deleteItemListener, confirmDeleteButtonListener, and load

To be stylistically consistent, consider abstracting the scroll listener into a separate function, like you're doing with the other listeners. (But the functions aren't event listeners themselves - they add event listeners, so maybe call them, eg, addDeleteConfirmListener)

Load quantity complication Juggling a dynamic number of items requested from the server seems a bit overcomplicated for my tastes. Unless you're expecting to have huge payloads, I think it'd make a bit more sense to fetch all items immediately, and then sort through what should be displayed at what time on the client. Sending all items at once will also reduce the visible delay for the user.

If you were to send all items immediately, consider interpolating the data into the HTML, rather than using a request after the page loads. Eg, you could have

<script type="application/json" class="items-data">
{"itemslist": [{"id": 191, "user_id": 26, "name": "name", "details": "details", "date": "2020-12-09"}]}
</script>

const items = JSON.parse(document.querySelector('.items-data').textContent);

This will make the page load more quickly for the user; assuming the app's been loaded before and you have a sensible cache policy, they won't have to wait for two round-trips to your server for the todos to be displayed, but only one.

Return early to avoid indentation hell This:

function deleteItemListener() {
  document.querySelector(".main").addEventListener("click", function(event) {
    if(event.target.nodeName === "I") {
      // big block of code

can be

function deleteItemListener() {
  document.querySelector(".main").addEventListener("click", function(event) {
    if (event.target.nodeName !== "I") {
      return;
    }
    // big block of code

or even

function deleteItemListener() {
  document.querySelector(".main").addEventListener("click", function(event) {
    if (event.target.nodeName === "I") {
      deleteItem(event.target);
    }

nodeName? It's not clear at a glance that a nodeName of i corresponds to the X icon. Consider giving the i a class that indicates what it is, eg delete-icon, allowing you to do e.target.matches('.delete-icon').

Use .closest when navigating to parents - the selector passed to .closest can be more informative as to which parent you're navigating to, and it's shorter than using .parentElement.parentElement:

const itemElement = event.target.closest('section');

innerHTML, textContent, and innerText

• Avoid innerText. It's almost never what you want - it has some confusing behaviors. Except in the very few cases where you need to retrieve text using innerText's peculiar formatting quirks, better to use .textContent or .innerHTML.

• Only use .innerHTML when setting or retrieving HTML markup. If all you want to set or retrieve is plain text, use .textContent instead; .textContent is faster, safer, and more semantically appropriate.

Related to the above:

innerHTML is a security risk with user input I can get your site to execute arbitrary code by entering the following description into a todo:

<img src onerror="alert('evil')">

I could disguise the markup in other text, then tell another user "Hey, try making a new todo with this description, you won't believe what happens next!" - and then steal their login credentials if they fall for it.

The problem is that you're using:

  itemNode.innerHTML =     
    `<p hidden class="id">${item.id}</p>
    ...
    <p class="details" name="details">${item.details}</p>

Direct concatentation of an HTML string like that should always be looked at suspiciously. If the interpolated value isn't 100% trustworthy, you probably have problems. To fix it, populate the untrustworthy values into the DOM by setting .textContent after creating the element, like this:

itemNode.innerHTML = `
  <p hidden class="id">${item.id}</p>
  <div name="delete-button" value="${item.id}" class="x-wrapper">
    <i class="far fa-times-circle"></i>
  </div>
  <h2 name="name" class="todo-item-name"></h2>
  <label for="details">Details:</label>
  <p class="details" name="details"></p>
  <p class="date">Deadline: ${item.date}</p>
`;
itemNode.querySelector('h2').textContent = item.name;
itemNode.querySelector('.details').textContent = item.details;

That's if the id and date are saved in a safe form in your database by your backend, which I'd expect they are, though the code isn't in the question - eg the id should be saved as a number, and the date should be saved as a unix timestamp or a Date. If they aren't and you can't for whatever reason, then you'll need to sanitize whatever comes from the user manually before saving it. If you can't do that either for some reason, then you'll have to insert the id and date properties into the DOM above by using DOM methods instead of by HTML concatenation - but it'd be better to be sure that they're in an expected, safe format ahead of time.

You could also enable direct concatenation of the name and details by sending them through a sanitizer before saving them to the database.