Javascript progress bar in succession

By Posted on

Problem

In the below code snippet, on click of a button, progress bars are added one by one. The bar shows up for 3 seconds. If you continuously click on the button, it will keep adding progress bars. The next bar will start after the previous bar finishes animating.

Is there anyway to make the code better?

;(function(window, document, undefined) {
    
    'use strict';

    var container = document.getElementsByClassName('container')[0];
    var button = document.getElementsByTagName('button')[0];
    var progressBar = document.getElementsByClassName("bar-container")[0];

    var promise = Promise.resolve();

    button.addEventListener('click', function() {
        var newProgressBar = progressBar.cloneNode(true);
        container.appendChild(newProgressBar);
        
        promise = promise.then(function() {
            return new Promise(function(resolve, reject) {
                setTimeout(function() {
                                      newProgressBar.classList.add('active');
                    setTimeout(function() {
                        resolve();
                    }, 3000);
                }, 0);
            });
        })
    });
    
    
})(window, document);
<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>Document</title>
    <style>
        .bar-container{
            width: 100%;
            background: #ccc;
            border: 1px solid #eee;
            margin-bottom: 10px;
        }
        .loader{
            height: 20px;
            background: #434334;
            width: 0px;
            transition: width 3s ease;
        }
        .active .loader{
            width: 100%;
        }
    </style>
</head>

<body>
    <div class="container"></div>

    <div style="display: none">
        <div class="bar-container">
            <div class="loader">
            </div>
        </div>
    </div>
    <button id="button">click me</button>
    
    
    <script src="script.js"></script>

</body>

</html>

Expand snippet

emphasized text

Solution

Your code already looks pretty clean. Good work! There are only a few things I would recommend changing.

  1. Don’t define timings in CSS that should be defined in JavaScript. In this specific case using a transition made it possible to simplify the code significantly, however in a more involved example it would become problematic. What happens if one progress bar takes 2 seconds to complete, and another takes 5?

  2. Instead of using getElementsByClassName('cls')[0], you can use querySelector('.cls') for more readable code. Though this can be slower, it really isn’t an issue unless it is being run thousands of times per second.

  3. There is a built in <progress> element that you may want to use.

  4. It is a good idea to wrap template elements in a <template> element.

Here’s a simple demo that defines the timing in the JavaScript. 

function Loader(element, time) {
  return new Promise(resolve => {
    let intervalId;
    function tick() {
      element.value = parseInt(element.value, 10) + 1
      if (element.value == element.max) {
        clearInterval(intervalId)
        resolve()
      }
    }
    setInterval(tick, time / element.max)
  })
}

let promise = Promise.resolve()
let container = document.querySelector('.container')
let template = document.querySelector('template')

function addProgress() {
  let progress = document.importNode(template.content, true)
  container.appendChild(progress)
  // progress is a document fragment, not the template element
  return container.querySelector('progress:last-child')
}

document.querySelector('button').addEventListener('click', () => {
  let bar = addProgress()
  promise = promise.then(() => Loader(bar, 3000))
})
progress {
  display: block;
  width: 100%;
}
<template>
  <progress max="100" value="0"></progress>
</template>
<div class="container"></div>
<button>Click me!</button>

Expand snippet

Redefining a standard type undefined is not a good idea and why redefine it you don’t use it.

  • Update It is a popular method to ensure undefined is indeed undefined, though the pattern is not completely safe and is really just a popular hack. ES6 allows you to ensure undefined as type using a constant, protecting it from mutation and avoiding accidental redefinition.

    It is better to use an alias for undefined to clearly show your intent. I personally use U but the use of safeUndefined would be a better option for collaborative work.

    const safeUndefined = ([])[0]; // first element of empty array will be undefined

    If you do not trust the Array then use the function return default const safeUndefined = (()=>{})();

Dont define what you do not use its just noise

Why redefine window which remains in scope even when redefined. You pass it and then ignore it when you set the timer events so why pass it. It may have made a little more sense if you did window.setTimeout rather than letting it default to the window object (not the one you passed)

And reject, you don’t use it, its not needed so why define it.

Use quotes consistently preferably “” don’t mix and match. You use single quotes for everything bar the progress bar class name. Though I can not work out why.

Use const in preference over var if you don’t plan on reassigning the variable.

The only good variable names are progressBar and newProgressBar,

  • buttonyes it is but what does it do? maybe addBarButton
  • container of what? maybe newBarContainer (I had to look further in the code to see what you did with it)
  • promise promising what? maybe progressComplete
  • resolve we all know that it what it does but what does it actually do, maybe startNextBar

The first progress bar does not work, you need to set its style correctly before you display. Dont set the loader class to 100% width in the style as that is happening when the page loads and can not be seen. Then when you want to start the progress add the active class to the the first child of the progress bar. This will stop the first bar from completing before it is seen.

The rewrite

;(function() {
  "use strict";

  const newBarContainer = document.getElementsByClassName("container")[0];
  const addBarButton = document.getElementsByTagName("button")[0];
  const progressBar = document.getElementsByClassName("bar-container")[0];
  var progressComplete = Promise.resolve();

  addBarButton.addEventListener("click", () => {
    const newBar = progressBar.cloneNode(true);
    newBarContainer.appendChild(newBar);

    progressComplete = progressComplete.then(() => {
      return new Promise((startNextbar) => {
        setTimeout(() => {
          newBar.children[0].classList.add("active");
          setTimeout(startNextbar, 1000);
        }, 0);
      });
    })
  });
})();
.bar-container {
  width: 100%;
  background: #ccc;
  border: 1px solid #eee;
  margin-bottom: 10px;
}

.loader {
  height: 20px;
  background: #434334;
  width: 0px;
  transition: width 1s ease;
}

.active  {
  width: 100%;
}
<button id="button">click me</button>
<div class="container"></div>

<div style="display: none">
  <div class="bar-container">
    <div class="loader">
    </div>
  </div>
</div>

Expand snippet

Leave a Reply

Your email address will not be published. Required fields are marked *