1
\$\begingroup\$

I have this JavaScript code that functions as intended, although as a beginner, I do not know of a shorter, more efficient way of writing it.

The code is a simple toggle that changes the title attribute of the <summary> HTML tag from hide to show when the user clicks on the <summary> to open the <details>.

I used loops since there are several of these in my page:

var i,l;

var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;

function detailsToggle() {
  for (i = 0; i < summElemLen; i++) {
    if (detailsElem[i].open === true) {
      summaryElem[i].title = 'hide';
    }
    else {
      summaryElem[i].title = 'show';
    }
  }
}

for (i = 0; i < detlElemLen; i++) {
  detailsElem[i].ontoggle = function() {detailsToggle()};
}

Is there a less verbose way of achieving the same function?

\$\endgroup\$
2
\$\begingroup\$
detailsElem[i].ontoggle = function() {detailsToggle()};

It's better to use addEventListener. The problem is that doing it this way replaces anything assigned to ontoggle, potentially blowing away any handler put by other code.

var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;

Personally recommending a var per variable. That way, you can easily add/remove lines without worrying about continuation. One such example is summElemLen ending with a ; and not a ,. This will cause detlElemLen to be defined on the global scope, and open to collision. This minor oversight can cause a lot of headache in the long term.

Also, using getElementsByTagName is very broad. Do you really want to take all elements? Or just a subset? If it's the latter, use a class name instead.

if (detailsElem[i].open === true) {

if statements expect a boolean. detailsElem[i].open is already a boolean. No need to do === true, it's redundant. true === true is always a true.

Lastly, one flaw in the code is that once you toggle one detail element, you toggle all summary titles regardless of whether they belong to the element you clicked.


The following is what I consider shorter, more explicit and takes advantage of the array API. We get all detail elements, loop through and attach a handler. Handler checks if a summary is present and only sets a title if there is one.

const slice = Array.prototype.slice

// querySelectorAll to select elements using a CSS selector.
// It returns an array-like structure (indexed, has length, no array API)
// To take advantage of array APIs, we use slice to convert to array.
const details = slice.call(document.querySelectorAll('.detail'))

// Use forEach to loop through each detail element found
details.forEach(detail => {

  // Add event handler when toggle is fired
  detail.addEventListener('toggle', event => {

    // Look for the summary with querySelector. It's like querySelectorAll
    // Except it returns either an element or null, instead of an array.
    const summary = detail.querySelector('summary')

    // No summary, do nothing.
    if(!summary) return;

    // Otherwise, assign title depending on detail state.
    summary.title = detail.open ? 'hide' : 'show'
  })
})
\$\endgroup\$
  • \$\begingroup\$ Why use Array.slice? Would it not be more concise to use the spread operator? eg const details = [...document.querySelectorAll(query)]; \$\endgroup\$ – Blindman67 Dec 4 '17 at 22:13
  • \$\begingroup\$ @Blindman67 old habits. \$\endgroup\$ – Joseph Dec 4 '17 at 22:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.