More efficient way to do this js code

JavaScript
1

I was wondering how could I improve my js code without overcomplicate it since I’m a beginner, thanks.
codepen: https://codepen.io/josemon322/pen/RwQrEBJ
html:

    <select id="selectDiv">
        <option hidden>Choose your div</option> 
        <option value="1">red div</option>
        <option value="2">blue div</option>
        <option value="3">black div</option>
    </select>
    <button id="showSelection">show selection</button>
    <div id="optionOne" class="hiddenDiv">content one</div>
    <div id="optionTwo" class="hiddenDiv">content two</div> 
    <div id="optionThree" class="hiddenDiv">content two</div>

css:

.hiddenDiv {display: none;}
#optionOne {
  width: 150px;
  height: 150px;
  background-color: red;
}
#optionTwo {
  width: 150px;
  height: 150px;
  background-color: blue;
}
#optionThree {
  width: 150px;
  height: 150px;
  background-color: black;
}

js:

document.getElementById("showSelection").addEventListener("click", showDivs)
function showDivs() {
    var divOne = document.getElementById("optionOne");
    var divTwo = document.getElementById("optionTwo");
    var divThree = document.getElementById("optionThree");
if (document.getElementById('selectDiv').value == 1) {
    divOne.style.display = "block";
    divTwo.style.display = "none";
    divThree.style.display = "none";
} else if(document.getElementById('selectDiv').value == 2) {
    divOne.style.display = "none";
    divTwo.style.display = "block";
    divThree.style.display = "none";
  }
  else if(document.getElementById('selectDiv').value == 3) {
    divOne.style.display = "none";
    divTwo.style.display = "none";
    divThree.style.display = "block";
  }
}
2

Without changing HTML and CSS, I’d write JS like this:

document.getElementById("showSelection").addEventListener("click", showDivs);
const selectEl = document.getElementById("selectDiv");
const divOne = document.getElementById("optionOne");
const divTwo = document.getElementById("optionTwo");
const divThree = document.getElementById("optionThree");

function showDivs() {
  divOne.style.display = "none";
  divTwo.style.display = "none";
  divThree.style.display = "none";
  
  if (selectEl.value == 1) {
    divOne.style.display = "block";
  } else if (selectEl.value == 2) {
    divTwo.style.display = "block";
  } else if (selectEl.value == 3) {
    divThree.style.display = "block";
  }
}

It’s better to get element references once, because DOM traversing is kind of slow.

1 Like
3

One thing that might be nice here would be the in-line if statement:

(boolean)? true: false

Basically you write the query, then a ‘?’ then what you want the value to be if true, then ‘:’ then what you want the value to be if false.

If you haven’t gotten to them yet you may want to look into them. Ended up being one of my favorite things when I got to that part in the class. Then you could write-

function showDivs() {
  divOne.style.display =  (selectEl.value == 1)? "block":"none";
  divTwo.style.display =  (selectEl.value == 2)? "block":"none";
  divThree.style.display = (selectEl.value ==3)? "block":"none";
}
1 Like
4

You can cache the button element value too, so you dont select it each time you need its value. You could also use switch block, because its more exact when comparing the value of single var multile times.

function showDivs() {
  var divOne = document.getElementById("optionOne");
  var divTwo = document.getElementById("optionTwo");
  var divThree = document.getElementById("optionThree");
  const selectValue = document.getElementById('selectDiv').value
  switch(selectValue) {
    case '1':
      divOne.style.display = "block";
      divTwo.style.display = "none";
      divThree.style.display = "none";
      break;
    case '2':
      divOne.style.display = "none";
      divTwo.style.display = "block";
      divThree.style.display = "none";
      break;
    case '3':
      divOne.style.display = "none";
      divTwo.style.display = "none";
      divThree.style.display = "block";
      break;
    default:
      divOne.style.display = "none";
      divTwo.style.display = "none";
      divThree.style.display = "none";
  }
}

It is also recommended to change styles via classes and not modify inline styles via element.style, for example in your code:

case '1':
      divOne.classList.remove('hiddenDiv')
      divTwo.classList.add('hiddenDiv')
      divThree.classList.add('hiddenDiv')
      break;

Should produce similar effect(altho it can be refiend even more, as we wont always need to add the class to every other element.

And then, how i would prolly approach this task:
I would use a single element as a color box, which would change its color based on the selected option. I would use classes to handle that.
HTML:

<div id="colorBox" class='hidden'>content one</div>

CSS:

#colorBox {
  width: 150px;
  height: 150px;
}
.hidden {
  display: none;
}
.black {
  background-color: black;
  color: white;
}
.blue {
  background-color: blue;
}
.red {
  background-color: red;
}

JS:

function showDivs() {
  const colorBox = document.getElementById('colorBox')
  const selectValue = document.getElementById('selectDiv').value
  switch(selectValue) {
    case '1':
      colorBox.className = 'red'
      break;
    case '2':
      colorBox.className = 'blue'
      break;
    case '3':
      colorBox.className = 'black'
      break;
    default:
      colorBox.className = 'hidden'
  }
}

You could also directly make the colors switch whenever the select valeu changes:

document.getElementById("selectDiv").addEventListener("change", showDivs)
1 Like
5

thanks a lot very handy answer.

6

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.