# Simple javascript toggle div function

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 icon = document.getElementById('right');
icon.onclick = function () {
}
}


Second code with two functions but does the same work:

function sidebar() {

},
function openbar() {
}

• Could you please take care of your indentation of code blocks and closing braces? This would enhance readability of your code a lot. Aug 15, 2020 at 10:20
• Welcome to Code Review! It would be helpful to know when the functions are called. Please edit your post to include relevant code that calls the functions. Include a snippet if you think it is appropriate. Aug 15, 2020 at 16:20

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:

# 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 =>
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>

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

// Grab the menu you want to open

}

// Secnario 2: if the menu you are trying to open isn't the currently open menu
}

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

display: block;
}
<div id="sidebar-container" class="js-menu-closed">
Sidebar 1
</div>

Sidebar 2
</div>

<button onclick="toggleMenu('#sidebar-container3')">Toggle sidebar 3</button>