0
\$\begingroup\$

I have three clickable <h1> elements in the nav bar of a static page I'm making as a practice project. I wrote code so when I click one, it lights up.

Since the page is static, it doesn't actually load another page, so it's just the button lighting up. When I click a different button, I want it to light up, and the previous one to turn off. I do this by adding and removing a class that alters the CSS of the element, however, I can't figure out a way of doing this more concisely than what I've already written, which adds the new class and removes any class for the other two elements. Surely there must be a more efficient way of doing this. I thought about a switch statement but I'm not quite sure how to use those yet.

function activeAbout(){
            var about = document.getElementById('about');
            about.setAttribute('class', 'playing');
            
            document.getElementById('poop').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
            }
        
        function activePoop(){
            var poop = document.getElementById('poop');
            poop.setAttribute('class', 'playing');
            
            document.getElementById('about').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
        }
            
        function activeDgech(){
            var dgech = document.getElementById('dgech');
            dgech.setAttribute('class', 'playing');
            
            document.getElementById('about').setAttribute('class', '');
            document.getElementById('poop').setAttribute('class', '');
        }
<div id='header'> 
       <h1 id='about' onclick="activeAbout()"><a href="#" >About</a></h1>
       <h1 id='poop' onclick="activePoop()"><a href="#">Poop</a></h1>
       <h1 id='dgech' onclick="activeDgech()"><a href="#">DGech</a></h1>
</div>

How can I refactor this more efficiently?

Edit: Added CSS code.

        #about{
            grid-column: 1;
            grid-row: ;
            justify-self: end;
            align-self: ;
            color: white;
            font-size: 60px;
            margin-top: 10px;
            
        }
        
        #poop{
            grid-column: 2;
            justify-self: center;
            color: white;
            font-size: 60px;
            margin-top: 10px;
        }
        
        #dgech{
            grid-column: 3;
            color: white;
            font-size: 60px;
            margin-top: 10px;
        }
        
        .playing{
            border-color: #ffc600;
            text-shadow: 0 0 0.5rem #ffc600;
        }

\$\endgroup\$

migrated from stackoverflow.com Dec 18 '17 at 2:58

This question came from our site for professional and enthusiast programmers.

1
\$\begingroup\$

Obviously the code works but it can be simplified in a couple ways.

Event Delegation

I would recommend using event delegation. Instead of adding click handlers in the HTML, a single click handler can be added in the JavaScript code to the element containing all the <h1> tags (or could be attached to document.body). Then refer to event.target to know which element was clicked. In the sample HTML given, the user might click on the anchor tag (specifically where the text is) or just to the right of it, which would signify clicking on the <h1> tag itself. To determine if the anchor tag was clicked or else the parentNode, .parentNode can be used.

One main advantage here is that the markup (i.e. HTML) doesn't need to contain any logic about handling the clicks - that can be separated and left in the JavaScript. This could be a separation of View and Controller code (a la MVC).

In the code snippet below, addEventListener is used to register a callback when the DOM is ready (via the DOMContentLoaded event) to be interacted with (i.e. querying, adding event handlers, etc.). Then it finds all the <h1> tags and iterates over them to set the class according to whether the element clicked was the current tag.

Spread operator

Note that the snippet below uses the EcmaScript-2015 (A.K.A. ES-6) spread operator to get an array from the NodeList returned from document.getElementsByTagName(). One could also use Array.from() instead of the spread operator.

//use spread operator to get array from node list of h1 elements
[...h1Elements] // can now use array methods like forEach

Bear in mind that IE and other browsers don't support the spread operator, so if you wanted a different syntax supported by those browsers, you could use apply

Array.prototype.forEach.apply(h1Elements, function(h1Element) {...

See an updated sample below:

document.addEventListener('DOMContentLoaded', function() {
  //store elements for use later - could use ES-6 keyword const instead of var
  var h1Elements = document.getElementsByTagName('h1');

  //add click handler to parent element - could also add to document.body
  document.getElementById('header').addEventListener('click', function(clickEvent) {
    //use spread operator to get array from node list of h1 elements
    [...h1Elements].forEach(function(h1Element) {
      //see if the anchor or the h1 was clicked
      const h1ElementClicked = (clickEvent.target.id || clickEvent.target.parentNode.id) === h1Element.id;
      if (h1ElementClicked) {
        h1Element.setAttribute('class', 'playing');
      } else {
        h1Element.setAttribute('class', '');
      }
    });
  });
});
#about {
  grid-column: 1;
  grid-row: ;
  justify-self: end;
  align-self: ;
  color: white;
  font-size: 60px;
  margin-top: 10px;
}

#poop {
  grid-column: 2;
  justify-self: center;
  color: white;
  font-size: 60px;
  margin-top: 10px;
}

#dgech {
  grid-column: 3;
  color: white;
  font-size: 60px;
  margin-top: 10px;
}

.playing {
  border-color: #ffc600;
  text-shadow: 0 0 0.5rem #ffc600;
}
<div id='header'>
  <h1 id='about'><a href="#">About</a></h1>
  <h1 id='poop'><a href="#">Poop</a></h1>
  <h1 id='dgech'><a href="#">DGech</a></h1>
</div>

\$\endgroup\$
  • \$\begingroup\$ This is definitely a very well-thought-out answer and I definitely appreciate it. I don't understand a couple parts of it, in particular the "[...h1Elements]" bit but that just tells me I still have a lot to get acquainted with in JS and only motivates me more to learn. Thank you. \$\endgroup\$ – Leo Dec 18 '17 at 16:15
  • \$\begingroup\$ Great- I added some context around that aspect. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 18 '17 at 16:22
1
\$\begingroup\$

You can also use this keyword

function activeAbout(sender){
            sender.setAttribute('class', 'playing');
            
            document.getElementById('poop').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
            }
        
        function activePoop(sender){
            sender.setAttribute('class', 'playing');
            
            document.getElementById('about').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
        }
            
        function activeDgech(sender){
            sender.setAttribute('class', 'playing');
            
            document.getElementById('about').setAttribute('class', '');
            document.getElementById('poop').setAttribute('class', '');
        }
<div id='header'> 
       <h1 id='about' onclick="activeAbout(this)"><a href="#" >About</a></h1>
       <h1 id='poop' onclick="activePoop(this)"><a href="#">Poop</a></h1>
       <h1 id='dgech' onclick="activeDgech(this)"><a href="#">DGech</a></h1>
</div>

\$\endgroup\$
1
\$\begingroup\$

One way to trim down your code would be to make all three links trigger the same function. Inside of this function, you'll want to strip the class from each of the elements, and then assign the playing class to the element that was clicked on. This can be done by passing in the target element as a function parameter.

Also note that .attr() is a jQuery method; you're probably looking for the property className instead.

This can be seen in the following:

function active(target) {
  var elements = document.getElementsByTagName('h1');
  for (var i = 0; i < elements.length; i++) {
    document.getElementsByTagName('h1')[i].className = "";
  }
  target.className = "playing";
}
<div id='header'>
  <h1 id='about' onclick="active(about)"><a href="#">About</a></h1>
  <h1 id='poop' onclick="active(poop)"><a href="#">Poop</a></h1>
  <h1 id='dgech' onclick="active(dgech)"><a href="#">DGech</a></h1>
</div>

Hope this helps! :)

\$\endgroup\$
  • \$\begingroup\$ I followed along the live edits lol but it still returns a TypeError. 'TypeError: Cannot set property 'className' of undefined' I appreciate the help though. \$\endgroup\$ – Leo Dec 18 '17 at 3:18
  • \$\begingroup\$ It would be best to reference elements within the for loop instead of querying the DOM each time. Also, did the OP have code referencing .attr()? I don't see that throughout the history... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 18 '17 at 8:17
1
\$\begingroup\$

This way you have to define the elements once and you won't have to change your HTML if you want to update anything.

let illuminatableElements = document.querySelectorAll('#poop, #about, #dgech');
for (var i = 0; i < illuminatableElements.length; i++) {
  illuminatableElements[i].addEventListener('click',function(){
    let activeElement = this; // bind this to a variable or you'll be using the wrong scope
    illuminate(activeElement);
  });
}

function illuminate(activeElement) { 
  for (var i = 0; i < illuminatableElements.length; i++) { // a forEach loop won't work (e.g. in MS Edge), because elements isn't an array
    illuminatableElements[i].classList.remove('playing');
  }
  activeElement.classList.add('playing');
}
a{
  color: black;
}
.playing a{
  color: orange;
}
<div id='header'>
  <h1 id='about'><a href="#">About</a></h1>
  <h1 id='poop'><a href="#">Poop</a></h1>
  <h1 id='dgech'><a href="#">DGech</a></h1>
</div>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.