trjhtr

I have <button>s that when pressed expand or collapse the contents of all the <details> within the document. The code works, but as a novice in JavaScript, I am not very confident in its performance and/or efficiency. Could you please review it.

expandDetBtn = document.getElementById("showBtn");
collapseDetBtn = document.getElementById("hideBtn");

expandDetBtn.addEventListener("click", function expandDetails(x) 
 var x = document.getElementsByTagName("details");
 var i;
 var len = x.length;
 for (i = 0; i < len; i++) 
 if (x[i].open == false) 
 x[i].setAttribute("open", "true");
 
 
);

collapseDetBtn.addEventListener("click", function expandDetails(x) 
 var x = document.getElementsByTagName("details");
 var i;
 var len = x.length;
 for (i = 0; i < len; i++) 
 if (x[i].open) 
 x[i].removeAttribute("open");
 
 
);

<button id="showBtn">Show details</button>
<button id="hideBtn">Hide details</button>

<details>
 <summary>Section 1</summary>
 <p>Text 1</p>
</details>

<details>
 <summary>Section 2</summary>
 <p>Text 2</p>
</details>

<details>
 <summary>Section 3</summary>
 <p>Text 3</p>
</details>

Performance-wise the code is fine. There is not really anything to improve. I have a few other comments about the code.

You are accessing DOM elements at the start, without checking if they are loaded. Usually they are not, which would cause an error here.

You are naming your handler function, but never refer to them. You can just use an anonymous function here. You also don't use the x parameter.

Don't use var, use let and const instead.

x is not a very descriptive name. It also overwrites the exiting parameter.

There is no need to save the length in a variable. You also don't need to declare i before the loop, since it's not used outside.

There is no need to check if they are already opened or closed. Opening one which is already open doesn't do anything, so just open them all.

Don't use "true" here .setAttribute("open", "true");. According to this.

Boolean attributes are considered to be true if they're present on the
element at all, regardless of their actual value; as a rule, you
should specify the empty string ("") in value

The outcome is the same, but it might lead people to believe that setting the value to "false" will remove it, which is not the case.

With all these changes, the opening look like this

expandDetBtn.addEventListener("click", function() 
 const elements = document.getElementsByTagName("details");
 for (let i = 0; i < elements.length; i++) 
 elements[i].setAttribute("open", "");
 
);

If you want to get more advanced you could use a single function, since they are very similar.

function toggleDetails(open) 
 for(element of document.getElementsByTagName("details")) 
 element.open = open;
 


expandDetBtn.addEventListener("click", toggleDetails.bind(null, true));
collapseDetBtn.addEventListener("click", toggleDetails.bind(null, false));