0
\$\begingroup\$

How can this be improved?

I'm looking particularly at the twice repeated tabButtons.forEach(button => {

const tabButtons = document.querySelectorAll('[data-tab-button]');
const tabContent = document.querySelectorAll('[data-tab-content]');

tabButtons.forEach(button => {
    button.onclick = () => {
        tabButtons.forEach(button => {
            button.classList.remove('text-black');
            button.classList.add('text-border-grey');
            button.classList.remove('border-b-2');
        });
        tabContent.forEach(content => {
            if(button.dataset.tabButton == content.dataset.tabContent) {
                content.classList.remove('hidden');
                button.classList.remove('text-border-grey');
                button.classList.add('text-black');
                button.classList.add('border-b-2');
            } else {
                content.classList.add('hidden');
            }
        });
    };
});
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to Code Review! It would benefit reviewers to have a bit more information about the code in the description. From the help center page How to ask: "You will get more insightful reviews if you not only provide your code, but also give an explanation of what it does. The more detail, the better." \$\endgroup\$ Aug 26, 2022 at 15:15
  • 3
    \$\begingroup\$ Showing your HTML and creating a complete, runnable snippet would be great. \$\endgroup\$
    – ggorlen
    Aug 26, 2022 at 17:06

1 Answer 1

1
\$\begingroup\$

I don't think the two loops of tabButtons.forEach is avoidable because these are executed at different times and both are necessary. The first is assigning a click handler to each button, which is necessary to make them interactive; and the second, on the click of a tab button, loops over them all to ensure that each has its visual state updated.

What I do find difficult to read about the code is all the .add and .remove calls. My suggestion is to consolidate these so that it is easier to understand which classes apply to which states and so that a particular type of element - tab button or content - can have its style updated in a single line of code.

First, I would put all of the class names for the active and inactive button states into string variables that can be easily and clearly referenced.

const activeButtonClassName = 'border-b-2 text-black';
const inactiveButtonClassName = 'text-border-grey'

If in the future we need to add or modify these styles, we now have a single place in which to do it.

Second, I would create a function which takes as argument a tabId which represents the attribute value which links a tab button (via its data-tab-button attribute) to a tab content (via its data-tab-content attribute). The function will do the work of visiting each tab button and tab content and setting the styling to active or inactive based on the value of tabId.

const activateTab = tabId => {
  tabButtons.forEach(tabButton => {
    const isActive = tabButton.dataset.tabButton === tabId;
    
    tabButton.className = isActive ? activeButtonClassName : inactiveButtonClassName;
  });

  tabContent.forEach(tabContent => {
    const isActive = tabContent.dataset.tabContent === tabId;

    tabContent.classList.toggle('hidden', !isActive);
  });
};

We still have the looping over each tab button and tab content, but I think the code that applies the styling is much more easily comprehensible.

I am using Element.className instead of Element.classList for the tabButton so that I can set the full string class attribute as either of the two variables we defined earlier. The assignment is clear: if active, assign active class; otherwise assign inactive class.

For the tabContent, I am using classList.toggle because all we are doing is toggling the hidden class depending on whether or not the content is active.

We will still need to assign a click handler to each tab button and a loop is still the best way to do this.

tabButtons.forEach(tabButton => {
  tabButton.addEventListener('click', (event) => {
    const activeTabId = event.target.dataset.tabButton;

    activateTab(activeTabId);
  });
});

I am using Element.addEventListener instead of Element.onclick because that's how I was raised, but I do not think that the differences are significant for this piece of code.

An additional benefit to having the tab activation logic in its own function is that it allows me to use JavaScript to initialize the application with the firast tab activated.

activateTab("tab1");

Putting it all together, the full example becomes:

const tabButtons = document.querySelectorAll('[data-tab-button]');
const tabContent = document.querySelectorAll('[data-tab-content]');

const activeButtonClassName = 'border-b-2 text-black';
const inactiveButtonClassName = 'text-border-grey'

const activateTab = tabId => {
  tabButtons.forEach(tabButton => {
    const isActive = tabButton.dataset.tabButton === tabId;
    
    tabButton.className = isActive ? activeButtonClassName : inactiveButtonClassName;
  });

  tabContent.forEach(tabContent => {
    const isActive = tabContent.dataset.tabContent === tabId;

    tabContent.classList.toggle('hidden', !isActive);
  });
};

tabButtons.forEach(tabButton => {
  tabButton.addEventListener('click', (event) => {
    const activeTabId = event.target.dataset.tabButton;

        activateTab(activeTabId);
  });
});

activateTab("tab1");
* {
  box-sizing: border-box;
  border-width: 0;
  border-style: solid;
  border-color: #e5e7eb;
}

button {
  font-family: inherit;
  font-size: 100%;
  font-weight: inherit;
  line-height: inherit;
  color: inherit;
  cursor: pointer;
  margin: 0;
  padding: 0;
}

[type="button"] {
  --webkit-appearance: button;
  background-color: transparent;
  background-image: none;
}

.border-b-2 {
  border-bottom-width: 2px;
}

.hidden {
  display: none;
}

.text-black {
    --tw-text-opacity: 1;
    color: rgb(0 0 0 / var(--tw-text-opacity));
}

.text-gray-800 {
  --tw-text-opacity: 1;
  color: rgb(31 41 55 / var(--tw-text-opacity));
}
<main class="text-gray-800">
  <button class="border-b-2" data-tab-button="tab1" type="button">
    Tab 1
  </button>
  <button data-tab-button="tab2" type="button">
    Tab 2
  </button>
  <button data-tab-button="tab3" type="button">
    Tab 3
  </button>

  <div data-tab-content="tab1">
    <p>
      Tab 1 content
    </p>
  </div>
  <div data-tab-content="tab2">
    <p>
      Tab 2 content
    </p>
  </div>
  <div data-tab-content="tab3">
    <p>
      Tab 3 content
    </p>
  </div>
</main>

I have also created a fiddle for reference.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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