1

I am trying to make this color "squares", which will change color after click. On first one it works fine, but when i click the others, the color change happens on the first one.

var image_tracker = 'red';

function changeColor() {
  var image = document.getElementById('red')
  if (image_tracker == 'red') {
    image.style.backgroundColor = "blue";
    image_tracker = 'blue';
  } else {
    image.style.backgroundColor = "red";
    image_tracker = 'red';
  }
}
<div id="red" onclick="changeColor()"></div>
<div id="red" onclick="changeColor()"></div>
<div id="red" onclick="changeColor()"></div>

there is no error messages, it just does not work as I would expect :D

Ken Y-N
  • 14,644
  • 21
  • 71
  • 114
Vojta Haberl
  • 21
  • 2
  • 3
    You have duplicate IDs, which is invalid markup and why it is not working as you would expect it to. Use classes instead. See [**this post**](https://softwareengineering.stackexchange.com/questions/127178/two-html-elements-with-same-id-attribute-how-bad-is-it-really). – Obsidian Age Aug 06 '19 at 21:46
  • another one. https://stackoverflow.com/questions/3607291/javascript-and-getelementbyid-for-multiple-elements-with-the-same-id – Murat Aras Aug 06 '19 at 21:48
  • 1
    All of the above, but also pay attention to function names - `getElementById` gets an *element* (not elements). `getElementsByClassName` (which is more suitable for your requirements) gets *elements* (plural). (And no, there isn't a `getElementsById` because there should only be 1 element with the unique Id) – Reinstate Monica Cellio Aug 06 '19 at 21:55
  • 1
    toggle a class to maintain state, be a lot easier than figuring it out with variables. – epascarello Aug 06 '19 at 21:57
  • Use `querySelectorAll`, and yes, use classes not id's – BugsArePeopleToo Aug 06 '19 at 22:01
  • It works exactly as expected as IDs must be unique, otherwise they are not really IDs. – Jon P Aug 07 '19 at 00:50

2 Answers2

0

IDs need to be unique, and document.getElementById() will only return the first element found. It doesn't know which element you clicked on.

Change your function to take image as a parameter. Then you can use this to refer to the element that was clicked on.

var image_tracker = 'red';

function changeColor(image) {
  if (image_tracker == 'red') {
    image.style.backgroundColor = "blue";
    image_tracker = 'blue';
  } else {
    image.style.backgroundColor = "red";
    image_tracker = 'red';
  }

}
<div onclick="changeColor(this)">1</div>
<div onclick="changeColor(this)">2</div>
<div onclick="changeColor(this)">3</div>
Barmar
  • 741,623
  • 53
  • 500
  • 612
0

Given that you have multiple "red" buttons that share common behaviour (ie that are clickable, behave in the same way, etc), consider "classifying" elements via a class rather than by an id:

var image_tracker = 'red';

/* Iterate all elements with "red" class */
for (const redElement of document.querySelectorAll('.red')) {

  /* Add click event handler */
  redElement.addEventListener('click', function() {
  
    /* Reuse existing click logic but based on redElement instead */
    if (image_tracker == 'red') {
      redElement.style.backgroundColor = "blue";
      image_tracker = 'blue';
    } else {
      redElement.style.backgroundColor = "red";
      image_tracker = 'red';
    }
  });
}
<!-- Define buttons as of the "red" class (and remove the "id") -->
<div class="red">Button 1</div>
<div class="red">Button 2</div>
<div class="red">Button 3</div>

You'll also see above that the inline "onclick" has been removed from the HTML in favour of the generally preferred addEventListener() based approach to event handling. Hope that helps!

Dacre Denny
  • 29,664
  • 5
  • 45
  • 65