1

Firstly, I know it would be more simple to achieve this using CSS but I am really trying to wrap my head around looping with JS as it is a new concept for me

What I want to happen is for the 'bg' class to loop through a number of background colours once

My code does not currently work would very much appreciate some direction :)

HTML

<div class="bg"></div>

CSS

.bg {
   width: 100%;
   height: 100%;
}

JS

var bg = document.getElementByClassName('bg');
var colours = ["#CCCDFF", "#BAC7E8", "#D9EEFF", "#BADFE8"];
  for (i = 0; i < colours.length; i++) {
    setInterval(change, 200); 
    function change() {
      bg.style.background = colours;
    }
  }
Burger Sasha
  • 187
  • 6
  • 17
  • Does this answer your question? [Simple way to synchronously execute code after setTimout() code is done](https://stackoverflow.com/questions/50918932/simple-way-to-synchronously-execute-code-after-settimout-code-is-done) – Rickard Elimää Dec 04 '19 at 22:03
  • They all run at 200 milliseconds. `200 * (i+1)` – epascarello Dec 04 '19 at 22:03

4 Answers4

5

There are 3 big problems with this line:

var bg = document.getElementByClassName('bg');
  1. The method is getElementsByClassName(). You missed an "s".
  2. .getElementsByClassName() returns a node list (collection) of all matching elements. The collection doesn't have a style property, only individual elements will. You'd have to extract an element from the collection first and then access it's style.
  3. .getElementsByClassName() returns a "live" node list, which hurts performance quite a bit. It's a very old API and shouldn't be used in 2019.

Next, because the interval timer will run continuously at the specified interval, the loop is not required. The repeating nature of the timer acts as a loop.

Next, in your CSS, you specify your element's size using percents, but percents have to be relative to something else, otherwise they won't work. If you want the element to be as big as the page, use vw and vh (Viewport Width and Viewport Height).

// Don't use `.getElementsByClassName()`
var bg = document.querySelector('.bg');
var colours = ["#CCCDFF", "#BAC7E8", "#D9EEFF", "#BADFE8"];
var index = 0; // Will keep track of which color to use

function change() {
  // If we have run out of colors, stop the timer
  if(index >= colours.length){ clearInterval(timer); }
  
  // Set the color and increment the index
  bg.style.backgroundColor = colours[index++];
}

// Start the timer but get a reference to it 
// so we can stop it later
var timer = setInterval(change, 200); 
.bg {
   width: 100vw;
   height: 100vh;
}
<div class="bg"></div>
Scott Marcus
  • 64,069
  • 6
  • 49
  • 71
1

You missed an s in getElementsByClassName, and since it returns multiple DOM elements, you need to select the first one with [0], or, as suggested by @ScottMarcus, simply use querySelector which will return the first element matching the selector you pass to it.

And when using setInterval inside a loop with the same delay, they're all going to trigger at the same time. Here is another approach using setTimeout:

var bg = document.querySelector('.bg');
var colours = ["#CCCDFF", "#BAC7E8", "#D9EEFF", "#BADFE8"];

chainColours(colours);

function chainColours(colours) {
  if (colours.length) {
    setColour(colours[0]);
    setTimeout(function() {
      chainColours(colours.slice(1)); // Repeat with all colours except the first one
    }, 200);
  }
}

function setColour(colour) {
  bg.style.background = colour;
}
.bg {
  width: 100px;
  height: 100px;
}
<div class="bg"></div>

Here, we're using a recursive function (a function which calls itself).

The first time we call it, we pass it the entire array of colours. The if condition will pass, because colours.length will be truthy (different than 0).

We set the bg colour to the first colour in that Array, using the setColour() function.

And then, using setTimeout, we wait 200ms to call the function again, but passing the Array without the first element, using slice.

It will go on until there are no elements left in that passed Array. Then, the if condition will not pass, and it will stop.

blex
  • 24,941
  • 5
  • 39
  • 72
  • This works perfectly thank you! There is quite a bit I don't understand within this code as I am still pretty fresh to learning JS but I will get researching :) – Burger Sasha Dec 04 '19 at 22:12
  • Good. I'll add some explanations below the code if that helps – blex Dec 04 '19 at 22:13
  • That would be amazing thank you so much! It will allow to accept this as the correct answer in a couple minutes :) – Burger Sasha Dec 04 '19 at 22:15
  • 1
    [`document.getElementsByClassName('bg')[0];` is just really, really bad code.](https://stackoverflow.com/questions/54952088/how-to-modify-style-to-html-elements-styled-externally-with-css-using-js/54952474#54952474) – Scott Marcus Dec 04 '19 at 22:20
  • @BurgerSasha You don't have to accept this answer, pick the one you prefer. Scott's is nice too – blex Dec 04 '19 at 22:24
1
const bg = document.querySelector('.bg');
const colours = ["#CCCDFF", "#BAC7E8", "#D9EEFF", "#BADFE8"];

(async () => {
  for (const [index, colour] of colours.entries()) {
    await new Promise(resolve => setTimeout(() => bg.style.background = colour, 200))
  }
})()

  • Use const (if variable should not be redefined) or let instead of var.
  • I replaced getElementByClassName with querySelector, as the former returns a list of elements and querySelector returns one
  • The self-invoking "async" function is used to make await keyword available. It awaits an asynchronous task.
  • The async task is the promise, which resolves after 200ms. Then the next async task is awaited.
adamz4008
  • 660
  • 4
  • 10
  • *getElementByClassName was replaced with querySelector, as the former returns a list of elements and querySelector returns one* <-- While I agree that `.querySelector()` should be used, your explanation for why is incorrect. Unfortunately, `.getElementsByClassName()` is still around. `.querySelectorAll()` would be the most appropriate analogy for it. – Scott Marcus Dec 04 '19 at 22:25
  • Also, the `async/await` code is redundant, since `setTimeout` is already asynchronous. – Scott Marcus Dec 04 '19 at 22:26
  • @ScottMarcus it "was replaced" not in browsers, but by me in the code example :). Sorry if that sounded confusing. Re: `setTimeout`, if we loop over every item and call `setTimeout` with 200ms, the background color will not update in 200ms intervals, which is what poster asked. It needs to synchronously wait for each 200ms setTimeout to complete, not run a bunch of setTimeout's asynchronously – adamz4008 Dec 04 '19 at 22:31
  • Yes, but your solution adds `async/await` unnecessarily because you kept the loop, which shouldn't be there in the first place. Removing the loop solves that problem. A repeating timer (recursive `setTimeout` or `setInterval`) is already essentially looping code. – Scott Marcus Dec 04 '19 at 22:33
  • How is it `async/await` added unnecessarily if it doesn't work without `async/await`? Why should a loop not be there in the first place if we need to iterate over an array? – adamz4008 Dec 04 '19 at 22:39
  • All that's needed is a way to increment a counter and a loop isn't needed for that. Just check out my working answer to see. No loop, no async/await and no promise. Your just reinventing what a timer already does. Let the timer do it's job. – Scott Marcus Dec 04 '19 at 22:49
  • Sure, it's not "needed". Programming languages are also not needed, but they make things more readable. This is why people who are used to modern JS tend to prefer promises over callbacks most of the time, even though they may not be absolutely "needed". Case in point, all the logic in my snippet is contained in two easy to read lines, and the flow doesn't skip around or rely on a global counter variable. I can also extend it a lot more easily than the callback-based approach, and make it play fine with any other async process. – adamz4008 Dec 04 '19 at 23:43
  • That is not to say I think your approach is "wrong", because it would do a good job in a certain context. It is just different than the approach I would take. Which is why I wrote it :). – adamz4008 Dec 04 '19 at 23:45
  • Well, I happen to be one of those people who are used to modern JS and I also happen to be a professional IT trainer, who understands that, while you may think that your code is 2 easy to read lines, the person asking this question will see it as 5 cryptic lines. My code, on the other hand, involves less resources and is actually 2 very simple lines. I'm also not sure if you understand your own code, since a Promise is a form of a callback. Your code also does rely on two global variables. I'm sorry, but there really isn't anything about your solution that is good. – Scott Marcus Dec 04 '19 at 23:50
  • 1. We don't need to worry about resources when looping over 4 elements. Readability matters. Premature optimization. 2. I understand my code. I also understand that a Promise is not a "form of callback" as you put it, but rather an object whose constructor takes a callback as an argument. – adamz4008 Dec 04 '19 at 23:58
  • I've already stated that I wrote it that way to be succinct, readable (non-spaghetti), and more maintainable than your callback-based approach. Not all professional IT trainers may agree that there are "anything good" about these qualities, but I bet many of them do :-P. – adamz4008 Dec 05 '19 at 00:04
0

setInterval will schedule a function to be called after a certain interval. It is non-blocking, so execution will proceed immediately after the function is queued. In this case, you're scheduling all four color changes to happen after 200 milliseconds, so the callbacks will be fired simultaneously.

Instead, you could do something like this: setInterval(change, 200 * i)

Brian
  • 1,860
  • 1
  • 9
  • 14
  • That makes sense thanks, this still doesn't run correctly though – Burger Sasha Dec 04 '19 at 22:07
  • Sorry, it looks like the problem was a typo in getElementByClassName. it's getElementsByClassName, which returns a nodelist, so you'll need to grab the first element in the list. An easier way to get an element would be: document.querySelector('.bg') – Brian Dec 04 '19 at 22:10