Simple javascript toggle div function

Question

I have written a code that triggers my div with another menu. I do not know which one is better and should be used. Can I get some feedback?

First code in one function:

function sidebar() {
var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      var icon = document.getElementById('right');
      menuSmall.style.display = "block";
      menu.style.display = "none";
      if(menu.style.display == "none") {
        icon.onclick = function () {
          menu.style.display = "block";
          menuSmall.style.display = "none";
}
}

Second code with two functions but does the same work:

function sidebar() {
      var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      menuSmall.style.display = "block";
      menu.style.display = "none";
        
    },
    function openbar() {
      var menu = document.querySelector('#sidebar-container');
      var menuSmall = document.querySelector('#sidebar-container2');
      menuSmall.style.display = "none";
      menu.style.display = "block";
    }

Answer 1

I think that is easier to write and understand the state change if you don't need to think about the edge cases.

In the first case, you already start in the state that you want and only need to invert the both states. In the second, you just need to "turn off" every states and "turn on" the one that you want to.

I think that is simplier if you think this way.

Two sidebars

I think that you can use DOMTokenList.toggle():

const sidebars = document.querySelectorAll('.sidebar');
const icon = document.getElementById('icon');

const toggle = el => el.classList.toggle('hidden');

icon.addEventListener('click',() => sidebars.forEach(toggle));

/* the display value of a div is block by default */
.hidden {
  display: none;
}

<!-- you don't need to find which sidebar is open in your js -->
<div class="sidebar">first</div>
<div class="sidebar hidden">second</div>
<span id="icon">icon</span>

Docs:

• DOMTokenList
• Element.classList
• DOMTokenList.forEach()

---

More than two sidebars

Maybe toggle can't be useful in this case. However, I think that datasets might help

const sidebars = document.querySelectorAll('.sidebar');
const btns = document.querySelectorAll('.btn');

const getSidebar = btn => btn.dataset.sidebar;

const hide = el => el.classList.add('hidden');

btns.forEach(btn =>
  btn.addEventListener('click', () => {
    const target = document.getElementById(getSidebar(btn));
    sidebars.forEach(hide);
    target.classList.remove('hidden');
  })
);

.hidden {
  display: none;
}

<div class="sidebar" id="first">sidebar 1</div>
<div class="sidebar hidden" id="second">sidebar 2</div>
<div class="sidebar hidden" id="third">sidebar 3</div>
<button class="btn" data-sidebar="first">1</button>
<button class="btn" data-sidebar="second">2</button>
<button class="btn" data-sidebar="third">3</button>

---

Answer 2

If I had to choose between your 2 methods, I would go with the second method.

You should always err on the side of readability and also the Single-responsibility Principle.

But if I were to suggest an alternative to your original code, I would propose the use of CSS classes.

function toggleMenu(selectedMenu) {
  // Grab the currently open menu
  var openMenu = document.querySelector('.js-menu-open');
  
  // Grab the menu you want to open
  var targetMenu = document.querySelector(selectedMenu);

  // Scenario 1: a menu is already open
  // Close the open menu.
  if(openMenu) {
    openMenu.classList.replace('js-menu-open', 'js-menu-closed');
  }
  
  // Secnario 2: if the menu you are trying to open isn't the currently open menu
  // Open the target menu.
  if(openMenu !== targetMenu) {
    targetMenu.classList.replace('js-menu-closed', 'js-menu-open');
  }
  
}

.js-menu-closed {
  display: none;
}

.js-menu-open {
  display: block;
}

<div id="sidebar-container" class="js-menu-closed">
  Sidebar 1
</div>

<div id="sidebar-container2" class="js-menu-closed">
  Sidebar 2
</div>

<div id="sidebar-container3" class="js-menu-closed">
  Sidebar 3
</div>

<button onclick="toggleMenu('#sidebar-container')">Toggle sidebar 1</button>
<button onclick="toggleMenu('#sidebar-container2')">Toggle Sidebar 2</button>
<button onclick="toggleMenu('#sidebar-container3')">Toggle sidebar 3</button>

This method essentially uses CSS classes as a kind of persistent state, which helps simplify your JS logic, and also reduces the repetition of your code (keeping it DRY) AND it is infinitely re-usable for as many menus as you'd like! :)

Other pluses is that you can now choose to have any one of your menu's open by default, and apply more performant animations in CSS.

Hope this helped.