Javascript progress bar in succession

Question

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>

emphasized text

\$\begingroup\$ Seems fine. BTW you can use setTimeout(resolve, 3000); \$\endgroup\$

woxxom
– woxxom

2017-09-24 16:09:58 +00:00
Commented Sep 24, 2017 at 16:09 — woxxom
– woxxom, Commented Sep 24, 2017 at 16:09

Gerrit0 · Accepted Answer · 2017-09-25 18:14:36Z

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

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?
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.
There is a built in <progress> element that you may want to use.
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>

how to implement point 1 using javascript? A small push in the right direction would do. — Rahul Arora
– Rahul Arora, Commented Sep 25, 2017 at 3:41
One way I can think of is, maintaining an array of seconds, and using it one by one. Since this is not a real time situation, mocking the data looks to be a good way. What do you suggest? — Rahul Arora
– Rahul Arora, Commented Sep 25, 2017 at 4:13
@RahulArora I updated with an example, you might want to modify this slightly. — Gerrit0
– Gerrit0, Commented Sep 25, 2017 at 18:15

Community · Accepted Answer · 2020-06-10 13:24:26Z

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>

Note that undefined is not redefined, the pattern the OP uses protects the code from using a redefined undefined value in the global scope. For example: { let undefined = true; console.log(undefined); (undefined => console.log(undefined))() } — Gerrit0
– Gerrit0, Commented Sep 24, 2017 at 21:06
Thank you for such a detailed feedback. I will tc of the variable names. — Rahul Arora
– Rahul Arora, Commented Sep 25, 2017 at 4:12
@Gerrit0 Yes it is redefined, As an argument it is a new variable, it could be any name. If you want to guarantee an undefined give it another name to clearly show intent, eg U, or safeUndefined as an alias for undefined and rather than pass it as an argument define it as a constant inside the function scope const U = ([])[0]; to protect it from mutation. — Blindman67
– Blindman67, Commented Sep 25, 2017 at 8:19

Stack Exchange Network

Javascript progress bar in succession

2 Answers 2

The rewrite

You must log in to answer this question.

Hot Network Questions

Javascript progress bar in succession

2 Answers 2

The rewrite

You must log in to answer this question.

Related

Hot Network Questions