0

The changeColor is being called in another function. I've tried this without a loop and it works just fine. The issue is it will only display on one box. I'm trying to get it to display on every box and I'm running into deadends. I've tried if and else if, it doesn't loop. What am I missing in my code? Why is it ignoring the loop? 

function changeColor() {

    var flash = document.querySelector(".side1");

    for (var i = 0; i < flash.length; i++) {
        flash[i].style.backgroundColor = 'green'

            setTimeout(function () {
                flash[i].style.backgroundColor = 'yellow'
            }, 3000);


            setTimeout(function() {
                flash[i].style.backgroundColor = 'red'
            }, 10000);

    } 

}
setInterval(changeColor, 80000000);
Improve this question
2
  • setInterval(changeColor, 80000000) ~ 22 hours Commented Nov 26, 2019 at 19:58
  • Hmmm. Every 22 hours? The browser window will need to be open that long ... what's your use case here? Commented Nov 26, 2019 at 20:01

2 Answers 2

Reset to default
3

document.querySelector only returns the first match, not an array. document.querySelectorAll will return an array of all matches.

EDIT Also, as @Drag13 points out below, all the timeouts will use the final value of i instead of the value it was when you first set the timeout. To fix this, you can use let i = 0 instead of var i = 0 to put i in block scope, meaning the current value will exist for all code inside that block, including your setTimeout callback.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

And timeout will also fails because they will be equal to to the flash.length always
0

As pointed out by others, document.querySelector only returns on element, not an array. You also have variable scoping issue and can use let to restrict the scope, but it is a relatively newer feature of the language. If compatibility is a concern you might want to find another solution like a closure, as suggested by Patrick Evans. Here is your example in a functional snippet. I sped things up a bit and fixed the scoping issue which Patrick Evans pointed out in the comments. TIL.

function changeColor() {
  var flash = document.querySelectorAll("div.flash")

  for (var i = 0; i < flash.length; i++) {
    flash[i].className = 'flash green'

    // create a closure which flashes yellow
    var flashYellow = (function() {
      var f = flash[i]
      return function() {
        f.className = 'flash yellow'
      }
    })()
    // create a closure which flashes red
    var flashRed = (function() {
      var f = flash[i]
      return function(){
        f.className = 'flash red'
      }
    })()

    setTimeout(flashYellow, 1000);
    setTimeout(flashRed, 2000);
  }
}
// change color immediately
changeColor();
// change color every three seconds after that
setInterval(changeColor, 3000);
div.flash {
  height: 25vh;
  width: 25vw;
}

div.green {
  background-color: green;
}

div.yellow {
  background-color: yellow;
}

div.red {
  background-color: red;
}
<div class="flash">text</div>
<div class="flash">other text</div>

Improve this answer

2 Comments

f is going to be the last assigned value in the loop by the time the timers trigger. Use let like you mention in the answer or if in an incompatible environment use a closure
@PatrickEvans you are right. I've updated the answer to include an example which illustrates that scoping issue.