Script for showing/hiding on click - how can I write this code cleaner?

Question

I have this simple code, but it's messy - how do I do it cleaner?

It works like this: If you click on the menu-btn1 the div shows up, and hide others, but if you click on menu-btn1 again it will hide as well.

let coll = document.getElementsByClassName('collection');
let dis = document.getElementsByClassName('display');
let drop = document.getElementsByClassName('drop');

document.getElementById("menu-btn1").addEventListener("click", function() {
  if(coll[0].style.display == 'none') {
    coll[0].style.display = 'block';
    dis[0].style.display = 'none';
    drop[0].style.display = 'none';
  } else {
    coll[0].style.display = 'none';
  }
});

<div class="menu">
    <span id="text"><span id="menu-btn1">collection</span></span>
</div>

<div class="collection" style="display:none;"></div>
<div class="display" style="display:none;"></div>
<div class="drop style="display:none;"></div>

There is nothing important in css really, only the grid positioning.

Answer 1

First I think that using document.querySelector seems much user friendly than using document.getElementsByClassName. Instead of let coll = document.getElementsByClassName('collection'); coll[0].xxx, we can just use document.querySeletor('collection').xxx.

So code can refactor a bit to

    let coll = document.querySelector('.collection');
    let dis = document.querySelector('.display');
    let drop = document.querySelector('.drop');

    document.getElementById("menu-btn1").addEventListener("click", function() {
        if(coll.style.display == 'none') {
            coll.style.display = 'block';
            dis.style.display = 'none';
            drop.style.display = 'none';
        } else {
            coll.style.display = 'none';
        }
    });

Next, I will place the DOM retrieval code into the event handler, to prevent unnecessary namespace pollution. Though maybe in your working code, you may use the coll for other handling, then it is okay to place there. In addition, I don't think one will change the variable of a DOM to other value, so I would use a constant instead.

    document.getElementById("menu-btn1").addEventListener("click", function() {
        const coll = document.querySelector('.collection');
        const dis = document.querySelector('.display');
        const drop = document.querySelector('.drop');
        if(coll.style.display == 'none') {
            coll.style.display = 'block';
            dis.style.display = 'none';
            drop.style.display = 'none';
        } else {
            coll.style.display = 'none';
        }
    });

However these changes does not resolve the fact that display: none is actually scattered in both HTML and Javascript. I think a better idea is just to place the code in one place. That is remove the styling in HTML.

To do so, I think there are possibly 2 approaches, one is the use of RxJS, another way is to use of data driven approach, e.g. React, Angular, Vue, ...

Data Driven Approach: Use of VueJS

Let's first have a look at How we can implement it in Data Driven Approach. I will use Vue for demonstration, as it is the simplest to setup among the three.

new Vue({
            el: "#app",
            template: `
            <div class="ui">
                <div class="menu">
                    <span id="text"><span @click="show = !show">collection</span></span>
                </div>

                <div class="collection" :style="styles">
                    Collection Inside
                </div>
            </div>
            `,
            data: function() {
                return {
                    show: false
                }
            },
            computed: {
                styles: function() {
                    return {
                        "display": this.show ? "block": "none"
                    };
                },
            },
        })

<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.5.17/vue.js"></script>
<div id="app">
  </div>

As you can see, the HTML code is now placed in Javascript as a template, and we make use of :style and @click in the template, instead of normal HTML. These are Vue specific syntax and is used in Vue for easy interaction between the Vue component Javascript and the template.

Event Driven Approach: RxJS

Another approach would be RxJS, that just work without a framework, so makes it more easy to adopt in a legacy web application.

rxjs.merge(
    rxjs.of(1), // so that it immediately triggers
    rxjs.fromEvent(document.querySelector("#menu-btn1"), 'click'),
)
    .pipe(
        rxjs.operators.scan((accum, value, index) => {
            return index % 2 == 1;
        }, false)
    ).subscribe(show => {
        const coll = document.querySelector(".collection");
        const value = show ? "block" : "none";
        coll.style.display = value;
    }

    )

<script src="https://cdnjs.cloudflare.com/ajax/libs/rxjs/6.6.3/rxjs.umd.min.js"></script>
<div class="menu">
    <span id="text"><span id="menu-btn1">collection</span></span>
</div>

<div class="collection">
    Collection Inside
</div>

Here is all the solution I think of, and it is left to you to decide which way is the cleanest code. To me, I just like the way RxJS is constructed, and do not need to maintain an extra data for storage.

Answer 2

There are some things that You could improve:

• Why do You treat a span element as a button when there is a more semantic solution which is a button element
• Another bad practice is an inline styling. It gives too much specificity to the element and it's hard to maintain that. Use stylesheets instead.
• Your variable names should be more descriptive. You want to make Your code reading experience as easy as it's possible so that other devs can easily understand it.
• Instead adding css styles using JS. Add a css class using JS and create those rules in the stylesheet.
• If I understand what You want to achieve correctly, Your code mechanics could be replaced using the event delegation pattern