4
\$\begingroup\$

I have made a simple To-Do Application just to get started with HTML/CSS/JavaScript and here is what I came up with. I have done what I can do at my best but still, I need some suggestions on my code, because I am new to Web-Development.

Well, here is what my code is doing:

This application helps others remember the works they have to do in nearby time.

So, the application has Add work a button, if the user clicks on that button then application prompts the user to add any work then it shows that work on screen and stores that work in localStorage and when the work is done then user can hover over the work and can click on the done button then the work gets removed from screen and from localStorage as well.

In addition to that, my JS code also searches for any work in localStorage at startup.

So if there is something wrong, or there is something which is not a recommended practice, or there is something which can be done in a better way, then please share that.

Here is the link to Github repo

Here is the whole source code:

// Instance of a div to add a work
let addButton = document.getElementById("addbutton");

// Instance of work list
let list = document.getElementById('workslist');

// Instance of title div
let title = document.getElementById('text');

// Number of works
let works = 0;

// Record of current works
let workTitles = [];

// function to add Typewrite effect
let type = function() {
    setTimeout(() => {
        title.innerHTML += 'T';
    }, 250);

    setTimeout(() => {
        title.innerHTML += 'o';
    }, 500);

    setTimeout(() => {
        title.innerHTML += '-';
    }, 750);

    setTimeout(() => {
        title.innerHTML += 'd';
    }, 1000);

    setTimeout(() => {
        title.innerHTML += 'o';
    }, 1250);

    setTimeout(() => {
        title.innerHTML += ' ';
    }, 1500);

    setTimeout(() => {
        title.innerHTML += 'A';
    }, 1750);

    setTimeout(() => {
        title.innerHTML += 'p';
    }, 2000);

    setTimeout(() => {
        title.innerHTML += 'p';
    }, 2250);

    setTimeout(() => {
        title.innerHTML += 'l';
    }, 2500);

    setTimeout(() => {
        title.innerHTML += 'i';
    }, 2750);

    setTimeout(() => {
        title.innerHTML += 'c';
    }, 3000);

    setTimeout(() => {
        title.innerHTML += 'a';
    }, 3250);

    setTimeout(() => {
        title.innerHTML += 't';
    }, 3500);

    setTimeout(() => {
        title.innerHTML += 'i';
    }, 3750);

    setTimeout(() => {
        title.innerHTML += 'o';
    }, 4000);

    setTimeout(() => {
        title.innerHTML += 'n';
    }, 4250);
}

// Calling type() to start TypeWriter effect.
type();

// Fetch works form localStorage
if (localStorage.getItem('works')) {
    workTitles = localStorage.getItem('works').split(',');
    workTitles.forEach(element => {
        let currentTitle = element;
        let entry = document.createElement('li');

        let div = document.createElement('div');
        div.className = 'works';
        div.id = 'works' + works;
        div.appendChild(document.createTextNode(currentTitle[0].toUpperCase() + currentTitle.slice(1)));
        div.title = 'Click to remove';

        entry.appendChild(div);

        let span = document.createElement('span');
        span.className = 'works remove'
        span.id = 'remove' + works;

        span.onclick = function() {
            let toBeDeleted = document.getElementById('item' + this.id.split('remove')[1]);
            if (workTitles.length != 1)
                workTitles.splice(this.id.split('remove')[1], 1);
            else
                workTitles.pop();
            toBeDeleted.style.transform = 'scale(0)';
            localStorage.setItem('works', workTitles);

            setTimeout(function () {
                list.removeChild(toBeDeleted);
            }, 1000);

            if (workTitles.length == 0) {
                document.getElementsByTagName('h4')[0].innerHTML = 'No Works to do.';
            }
        }

        span.appendChild(document.createTextNode('Done'));

        entry.appendChild(span);
        entry.id = 'item' + works;

        let listitem = list.appendChild(entry);

        listitem.onclick = function() {
            if (workTitles.length != 1)
                workTitles.splice(this.id.split('remove')[1], 1);
            else
                workTitles.pop();
            listitem.style.transform = 'scale(0)'
            localStorage.setItem('works', workTitles);

            setTimeout(() => {
                list.remove(listitem);
            }, 1000);

            if (workTitles.length == 0) {
                document.getElementsByTagName('h4')[0].innerHTML = 'No Works to do.';
            }
        }

        // Show animation when new work get added
        let newWork = document.getElementById('item' + works);
        setTimeout(function () {
            newWork.style.transform = 'scale(1)';
        }, 1);

        works++;
    })
} else {
    document.getElementsByTagName('h4')[0].innerHTML = 'No Works to do.'
}

// ClickListener for addButton
addButton.addEventListener('click', function() {
    let currentTitle = prompt('Add your work here');

    // Check if value is not null
    if (currentTitle) {
        let entry = document.createElement('li');

        let div = document.createElement('div');
        div.className = 'works';
        div.id = 'works' + works;
        div.appendChild(document.createTextNode(currentTitle[0].toUpperCase() + currentTitle.slice(1)));

        entry.appendChild(div);

        let span = document.createElement('span');
        span.className = 'works remove'
        span.id = 'remove' + works;

        span.onclick = function() {
            let toBeDeleted = document.getElementById('item' + this.id.split('remove')[1]);
            if (workTitles.length != 1)
                workTitles.splice(this.id.split('remove')[1], 1);
            else
                workTitles.pop();
            toBeDeleted.style.transform = 'scale(0)';
            localStorage.setItem('works', workTitles);

            setTimeout(function () {
                list.removeChild(toBeDeleted);
            }, 1000);

            if (workTitles.length == 0) 
                document.getElementsByTagName('h4')[0].innerHTML = 'No Works to do.';

        }

        span.appendChild(document.createTextNode('Done'));

        entry.appendChild(span);
        entry.id = 'item' + works;

        let listitem = list.appendChild(entry);

        listitem.onclick = function() {
            if (workTitles.length != 1)
                workTitles.splice(this.id.split('remove')[1], 1);
            else
                workTitles.pop();
            listitem.style.transform = 'scale(0)'
            localStorage.setItem('works', workTitles);

            setTimeout(() => {
                list.remove(listitem);
            }, 1000);

            if (workTitles.length == 0) {
                document.getElementsByTagName('h4')[0].innerHTML = 'No Works to do.';
            }
        }

        // Show animation when work get added
        let newWork = document.getElementById('item' + works);
        setTimeout(function () {
            newWork.style.transform = 'scale(1)';
        }, 1);

        works++;
        workTitles.push(currentTitle);
        document.getElementsByTagName('h4')[0].innerHTML = 'Works to do:'
        localStorage.setItem('works', workTitles);
    }
})
body {
    background-image: url(https://i.stack.imgur.com/usyUd.png);
    font-family: 'Roboto', cursive,  Arial, Helvetica, sans-serif;
}

li {
    list-style: none;
    transition: 1s;
    transform: scale(0);
}

span {
    margin: 0%;
    padding: 0%;
}

h4 {
    text-align: center; 
    margin-top: 100px; 
    font-family: Montserrat, 'Consolas';
    font-size: 3ch;
    color: #2B2B52;
}

#title {
    margin-left: 30px;
    margin-right: 30px;
    margin-top: 30px;
    padding-top: 10px;
    padding-bottom: 10px;
    border-radius: 30px;
    text-align: center;
    font-family: 'Roboto Mono', cursive;
    font-size: 4vh;
    background-color: #8B78E6;
    color: #F5F5F5;
}

#cursor {
    color: #f5f5f5;
    animation: blink-cursor 1s step-end infinite;
}

#subtitle {
    font-family: Comfortaa, cursive;
    text-align: center;
    font-size: 1.2em;
    color: #26F9D8;
    font: bold;
}

.works {
    margin: 10px;
    transition: 0.3s;
    color: #FFF;
    padding: 10px;
    display: inline-block;
    background-color: #FB6F6F;
    border-radius: 16px;   
}

.works:hover {
    transform: scale(1.1);
    box-shadow: 0px 5px 15px 2px rgba(0, 0, 0, 0.2);
}

.works:hover~.remove {
    transform: scale(1);
}

.remove {
    transition: 0.5s;
    transform: scale(0);
    cursor: pointer;
}

.remove:hover {
    transform: scale(1.1); 
}

#addbutton {
    display: inline-block;
    padding-right: 30px;
    padding-left: 30px;
    transition: 0.5s;
    color: #212121;
    border-radius: 16px;
    cursor: pointer;
}

#addbutton:hover {
    background-color: #212121;
    color: #F5F5F5;
}

@keyframes blink-cursor {
    from, to {opacity: 1;}
    50% {opacity: 0;}
}
<!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">
    <link rel="stylesheet" type="text/css" href="main.css">
    <title>To-Do</title>

    <!-- Fonts -->
    <link href="https://fonts.googleapis.com/css?family=Comfortaa" rel="stylesheet">
    <link href="https://fonts.googleapis.com/css?family=Poor+Story" rel="stylesheet">
    <link href="https://fonts.googleapis.com/css?family=Roboto" rel="stylesheet">
    <link href="https://fonts.googleapis.com/css?family=Roboto+Mono" rel="stylesheet">
    <link href="https://fonts.googleapis.com/css?family=Montserrat:700" rel="stylesheet">
</head>
<body>
    <div id="container">
        <div id="title">
            <b><span id="text"></span><span id="cursor">|</span></b>
        </div>

        <h4>Works to do:</h4>

        <ul id="workslist"></ul>

        <div style="text-align:center; transition: 1s;">
            <div id="addbutton"><p><b>Add Work</b></p></div> 
        </div>
    </div>
    
    <!-- JS Script -->
    <script src="main.js"></script>
</body>
</html>

\$\endgroup\$
  • 3
    \$\begingroup\$ I've linked to a snapshot of your repository (the latest commit), so that if you commit changes later, the question still makes sense. \$\endgroup\$ – 200_success Oct 18 '18 at 14:36
2
\$\begingroup\$

If you notice yourself repeating a particular pattern often, there's probably a way to write it more efficiently (DRY, do not repeat yourself). For example your type function. It could also be more loosely coupled with the rest of your application (if you want to use the type-writing effect elsewhere), and easier to maintain by making it more generic:

(See Batter's answer for a more simple way to write the type function)

// function to add Typewrite effect
const type = function(el, s, interval) {
  const typeInterval = setInterval(function() {
    el.textContent += s.charAt(el.textContent.length);
    if (el.textContent.length == s.length) clearInterval(typeInterval);
  }, interval);
};

type(title, "To-do application", 250);

The type function rewritten, you pass 3 parameters. The element you want to update, the string you want to write and the interval, which determines the speed.

The interval is being cleared as soon as the string written in the dom equals the string passed as param.

Also you might want to check when to use const, and when to use let. At the top of your document you are creating references to DOM elements, such as the addbutton which won't change, your might want to use const.

To give you an idea how to make it more managable I did some refactoring of the JS:

// Instance of a div to add a work
const addButton = document.getElementById("addbutton");

// Instance of work list
const list = document.getElementById("workslist");

// Instance of title div
const title = document.getElementById("text");

// Number of works
let works = [];

// function to add Typewrite effect
const type = function(el, s, interval) {
  const typeInterval = setInterval(function() {
    el.textContent += s.charAt(el.textContent.length);
    if (el.textContent == s) clearInterval(typeInterval);
  }, interval);
};

const renderWorks = function() {
  list.innerHTML = "";
  if (works.length > 0) {
    works.forEach(function(workTitle, i) {
      let li = document.createElement("li");
      let div = document.createElement("div");
      let span = document.createElement("span");

      div.className = "works title";
      div.id = "works" + i;

      span.className = "works remove";
      span.id = "remove" + i;
      span.textContent = "remove";

      span.addEventListener("click", () => removeWork(i));

      div.textContent = workTitle.toUpperCase();

      list
        .appendChild(li)
        .appendChild(div)
        .appendChild(span);
    });
  } else {
    // No works to do
  }
};

const removeWork = function(i) {
  works.splice(i, 1);
  setWorks();
  renderWorks();
};

const addWork = function(work) {
  works.push(work);
  setWorks();
  renderWorks();
};

const getWorks = function(callback) {
  works =
    localStorage.getItem("works").length > 0
      ? localStorage.getItem("works").split(",")
      : [];
  callback();
};

const setWorks = function() {
  localStorage.setItem("works", works.toString());
};

const promptNewWork = function() {
  const work = prompt();
  addWork(work);
};

type(title, "To-do application", 250);
addButton.addEventListener("click", promptNewWork);
getWorks(renderWorks);

Note: I didn't add all the right classname, so disable the CSS to see it working.

Breaking your code into small bits with a single purpose often improves maintainability of the code. Seperate functions for each task. Note there are probably even more efficient ways then this, but I hope it points you in a direction ;)

Also note that this particular code rewrites the entire list on an addition/deletion, from an performance perspective this might not be optimal.

\$\endgroup\$
  • 1
    \$\begingroup\$ I can understand your type() function, but that's a little more complex than the one by Batters \$\endgroup\$ – Zlytherin Oct 18 '18 at 15:22
  • \$\begingroup\$ Well, I'll take care of const next time thanks \$\endgroup\$ – Zlytherin Oct 18 '18 at 15:22
  • 1
    \$\begingroup\$ @DudeCoder I've added a reference to his answer. \$\endgroup\$ – Titsjmen Oct 18 '18 at 17:21
  • \$\begingroup\$ Cool @Titsjmen. \$\endgroup\$ – Zlytherin Oct 19 '18 at 3:14
2
\$\begingroup\$

As you've asked if there's anything that can be done in a better way, here's my suggestion for your type() function:

let type = (target, delay, text) => {
    if (text.length === 0) {
        return;
    }

    target.innerHTML += text[0];
    return setTimeout(() => type(target, delay, text.slice(1)),
        delay);
};

Rather than explicitly list the delay for each letter, I've created a function that accepts a DOM element, a delay time and a text string. On each call, it adds the first letter in the text string to the innerHTML of your target element and then calls itself recursively with a delay, having removed the first letter from the text argument. When there's no text left (i.e. text.length === 0) the function is done and can return.

I may not have explained it brilliantly but if you called it as:

type(title, 250, 'To-do Application')

it should give you exactly the same result as your current code. It can then be reused with any DOM element, delay and text of your choosing should you wish to.

\$\endgroup\$
  • \$\begingroup\$ Thanks bro for the information, You have explained it amazingly, well, I'm not accepting your answer as of now because there can be some more good answers, but I've upvoted your answer at least. ;) \$\endgroup\$ – Zlytherin Oct 18 '18 at 14:54

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.