Javascript style.display is not updating the style when I use my function-CodePudding

I am trying to make a function that when you click a button, it removes a div, and adds another div, then when you press that button again, it'll remove that div and that a different div. this goes around 4 times, until i reach the end, then when i press the button again it should go back to the beginning. what am i doing wrong?

let switchData = (selected, removed1, removed2, removed3) => {
    if(!document.getElementById("show-next-data")) {
        return;
    }
    document.getElementById("show-next-data").addEventListener("click", ()=> {
        let infoData = [selected, removed1, removed2, removed3];   

        document.getElementById(selected).style.display = "block";
        /*for (let i = 0; i < infoData.length; i  ) {
            document.getElementById(infoData[i]).style.display = "none";
        }*/
        document.getElementById(removed1).style.display = "none";
        document.getElementById(removed2).style.display = "none";
        document.getElementById(removed3).style.display = "none";

    });
}
switchData("dog-care", "cat-care", "youtube-media", "table-data");
switchData("cat-care", "dog-care", "youtube-media", "table-data");
switchData("youtube-media", "cat-care", "dog-care", "table-data");
switchData("table-data", "cat-care", "youtube-media", "dog-care");

<div id="info-content">
    <div id="dog-care">
        <h1 id="animal-info">How To Take Care Of My Dog:</h1>
        <h4 id="animal-info">Make sure your dog is well fed 
        </h4>
    </div>
    <div id="cat-care">
        <h1 id="animal-info">How To Take Care Of My Cat:</h1>
        <h4 id="animal-info">Make sure your cat is well fed 
        </h4>
    </div>
    <div id="youtube-media">
        <iframe width="200%" height="300%" src="https://www.youtube.com/embed/Yzv0gXqoCkc" title="YouTube video player" 
        frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" ></iframe>
    </div>
    <div id="table-data">

        <h1>table goes here</h1>

    </div>

    <h1 id="show-next-data">︾</h1>
  </div>

CodePudding user response：

Make your life easier by writing less and doing more:

const divs = document.querySelectorAll("#info-content>div");
var idx=0;
divs.forEach(nextDiv); // step through all divs once to hide them all but the first ...
document.getElementById("show-next-data").onclick=nextDiv;
function nextDiv(){
 divs[idx].style.display="none";
 divs[idx=(idx 1)%divs.length].style.display="";
};

<div id="info-content">
  <div id="dog-care">
    <h1 id="animal-info">How To Take Care Of My Dog:</h1>
    <h4 id="animal-info">Make sure your dog is well fed
    </h4>
  </div>
  <div id="cat-care">
    <h1 id="animal-info">How To Take Care Of My Cat:</h1>
    <h4 id="animal-info">Make sure your cat is well fed
    </h4>
  </div>
  <div id="youtube-media">
    <iframe width="200%" height="300%" src="https://www.youtube.com/embed/Yzv0gXqoCkc" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"></iframe>
  </div>
  <div id="table-data">
    <h1>A table goes here</h1>
  </div>

  <button id="show-next-data">︾</button>
</div>

The function nextDiv() shows the next div starting from idx (the index number within the collection divs (found with the selector #info-content>div). The current div is hidden, idx is increased and the next div is displayed by reseting its .style.display attribute to "" (an empty string).

CodePudding user response：

The issue is that you add event listeners instead of actually changing the anything on the page. Calling the function switchData() adds an EventHandler to show-next-data. When you click on that div, the code inside the EventHandler gets fired. Nothing happens before this.

That doesn't seem to be what you want to do though. Because when you call switchData() several times, you simply add more EventHandlers. And you can't be certain in which order the EventHandlers are fired, so your code becomes unpredictable.

What you probably want to do instead, is to just get rid of the event handler. Your function would look like this then:

let switchData = (selected, removed1, removed2, removed3) => {
    if(!document.getElementById("show-next-data")) {
        return;
    }
    let infoData = [selected, removed1, removed2, removed3];   

    document.getElementById(selected).style.display = "block";
    /*for (let i = 0; i < infoData.length; i  ) {
        document.getElementById(infoData[i]).style.display = "none";
    }*/
    document.getElementById(removed1).style.display = "none";
    document.getElementById(removed2).style.display = "none";
    document.getElementById(removed3).style.display = "none";
}

Now calling the function actually does something, but clicking the show-next-data div doesn't of course. So you would still need an EventHandler (btw EventHandler & EventListener means the same thing here). So you would add the EventHandler outside of the switchData() function and then call switchData from inside the EventHandler.

The difference to your current code is that the switchData function currently adds new EventListeners, that will fire when the div is clicked. I would suggest adding a single EventHandler outside the function, that then calls the function. That is probably what you wanted in the first place and doesn't create new EventHandlers with every function call making the code very unpredictable.

The only remaining question would be to figure out when you want to show what data. Currently you just hard-coded it with calling the function 4 times. That's bad design, cause it makes it very difficult to go back and change something (like adding a new section, removing an old section, etc.

For that you could loop through all children nodes of "info-content" div and check which one is currently visible and which ones aren't and then show the next one or something.

Before ending this long reply I want to give you some last tips:

"show-next-data" is suppposed to be a button, right? Make it a button. Don't make it an h1 header. There might actually be some issue with the browser not accepting an onclick even for headers, but I didn't test that. But please make it a button element anyways, to make your code more readable.
Don't declare a function wit "let". "let" is meant for variables that you want to change later again. For functions you should better use "const" (same as let, except it doesn't let you change the variable later) or normal function syntax (i.e. "function switchData(...) { ... }" or "const switchData = (...) { ... }").
Instead of "removed1, removed2, removed3", you might want to change the function later to hide 4 divs or only 2 divs if your website layout changes. So a better way to make your function, is to use the rest parameter: switchData(selected, ...removed). "removed" would now be an array. You would still call switchData the way you do rn, i.e. switchData("dog-care", "cat-care", "youtube-media", "table-data"); would still be the way to call the function, but in the function body you would have removed == ["cat-care", "youtube-media", "table-data"] now. Then you can just loop through the array and hide each element like you do now. This lets you easily change the amount of parameters without having to change your entire function.