Нормальный скрипт для новичка в JS?

Question

[N!crO]

Помогите доработать, если что-то не так.
Скрипт для оживления табов.

let tabsBtn = document.querySelectorAll('.services__tabs-item');
let tabsContent = document.querySelectorAll('.services__item');

for (let i = 0; i < tabsBtn.length; i++) {
    tabsBtn[i].addEventListener('click', function(evt) {
        evt.preventDefault();
        compareIndex(i);
    }); 
}

function compareIndex(indexBtn) {
    for (let j = 0; j < tabsContent.length; j++) {
        hideTabs(j);
        if (indexBtn == j) {
            tabsBtn[indexBtn].classList.add('services__tabs-item--active');
            tabsContent[j].classList.add('services__item--active');
        }
    } 
}

function hideTabs(tabsActive) {
    tabsBtn[tabsActive].classList.remove('services__tabs-item--active');
    tabsContent[tabsActive].classList.remove('services__item--active');
}

Comments

[Dima Polos]

А что не так? Если не работает - напишите, что не работает, если хотите спросить, как оно со стороны, то другое дело) Чтоб Вам скорее помогли закиньте это сюда: https://jsfiddle.net/ Иначе тем, кто захочет ответить придется самим копировать Ваш JS, потом самим выдумывать разметку под Ваш скрипт и потом что-то советовать. Вопрос дооформите, плиз, и ответ будет адекватным и вовремя.

[Dima Polos]

Самое первое, что вижу, Вы используете иттератор для вызова функции по клику, это не будет работать, так как иттератор пройдет по циклу и его значение к тому моменту, как кто-то кликнет куда-то будет уже максимальное, и функция всегда будет вызываться с одним и тем же значением параметра. Вместо этого либо надо вызывать с this, либо передавать объект-события в функцию., смотря, что в итоге надо сделать.

[N!crO]

Dima Polos, скрипт работает, просто хотел узнать ваше мнение.

Answer 1

[Николай Шабалин]

1) Возможно у табов и контента есть один родитель, какой-нибудь .services, тогда лучше через родителя искать детей

let tabsWrapper = document.querySelector('.services');
  let tabsBtn = tabsWrapper.querySelectorAll('.services__tabs-item');
  let tabsContent = tabsWrapper.querySelectorAll('.services__item');

Если всё-таки нет общего родителя, то можно закешировать document. Это всё делается для минимизации трогания DOM, чем меньше тем лучше.

2) Вы в цикле проходитесь по табам и вешаете слушателей. Это не самый лучший вариант, лучше было бы делегировать событие - подробнее тут. По факту вы всегда вешаете одного слушаетеля, а не сколько-то в завимисость от того сколько табов.

3) Если уж es6, то до конца. Применяйте стрелочные функции.

tabsBtn[i].addEventListener('click',  evt => {
       evt.preventDefault();
       compareIndex(i);
   });

4) Не сокращайте название переменных (куда вы деваете время, которое сэкономили на напечатание). Сокращением пускай занимаются роботы - минификаторы, обфускаторы - Вы пишите для людей

evt => event
tabBtn => tabButton
etc.

5) при каждом клике Вы зачем-то проходитесь по всем табам, хотя активным может быть только один таб. Сохраняйте активный таб и как только кликаете на новый, с активного убираете css-классы, а после назначаете кликнутый таб активным.

Comments

[Александр Таратин]

4 - ну... такой себе совет - вкусовщина.
Всегда ограничивался одной буквой e для event

[Николай Шабалин]

Александр Таратин, так оно и есть - это чистейшая вкусовщина, но я редко стал надеяться, что меня поймут и поэтому придерживаюсь правила - "чем очевиднее, тем лучше".

Можно мы с Вами и поймём друга друга, но новичок может и не понять, что значит, что лучше от такой системы нейминга избавиться и использовать более очевидную.

[Dima Polos]

Стрелочная функция в addEventListener приведет к тому, что в this будет window, ее прикол в сохранении контекста, а не в экономии на слове function)

[Николай Шабалин]

Dima Polos, я и не писал, что стрелочная функция экономит на слове function. У меня пункт озаглавляется - "Если уж es6, то до конца. Применяйте стрелочные функции." Про скоращения это 4ый пункт - где про нейминг уже.

[Dima Polos]

Николай Шабалин, да, я понял, но если писать стрелку в обработке события теряем контекст вызова, так что лучше новичкам не злоупотреблять стрелками, а то потом если в обработке события, например, ссылаться на контекст вызова, то будут сплошные андефайнды :)
Просто хотел Ваши советы дополнить уточнением этого нюанса.

[N!crO]

Переделал по вашим указаниям, пришлось использовать parentNode из-за того, что я немного с разметкой налажал, а переверстывать лень. Скрипт рабочий.

var tabsWrapper = document.querySelector('.services .container');
var tabsBtn = tabsWrapper.querySelectorAll('.services__tabs-item');
var tabsContent = tabsWrapper.querySelectorAll('.services__item');

var activeBtnTab = tabsWrapper.querySelector('.services__tabs-item--active');
var activeContentTab = tabsWrapper.querySelector('.services__item--active');


tabsWrapper.addEventListener('click', function(event)  {

    event.preventDefault();
    var target = event.target;

    if (target.parentNode.classList.contains('services__tabs-item')) {

        for (var i = 0; i < tabsBtn.length; i++) {

            if (tabsBtn[i] === target.parentNode) {
                activeBtnTab.classList.remove('services__tabs-item--active');
                activeBtnTab = target.parentNode;
                activeBtnTab.classList.add('services__tabs-item--active');

                activeContentTab.classList.remove('services__item--active');
                activeContentTab = tabsContent[i];
                activeContentTab.classList.add('services__item--active');

                return;
            }

        }

    }

});

Answer 2

[Dima Polos]

let tabsBtn = document.querySelectorAll('.services__tabs-item');
let tabsContent = document.querySelectorAll('.services__item');

for (let i = 0; i < tabsBtn.length; i++) {
    tabsBtn[i].addEventListener('click', function(evt) {
        evt.preventDefault();
        for (let j = 0; j < tabsContent.length; j++) {
             tabsBtn[j].classList.remove('services__item--active');
             tabsContent[j].classList.remove('services__tabs-item--active');
         }
        let index = tabsBtn.indexOf(this);
        this.classList.add('services__tabs-item--active');
        tabsContent[index].classList.add('services__item--active');
    }); 
}

По идее, если я все правильно понял, то такой код должен работать.
Суть в том, что иттератор передавать в прослушку события бессмысленно, цикл пройдет и закончится, значение иттератора будет больше длины массива, и это произойдет за доли секунды, а когда пользователь кликнет по табу вызовется функция с иттератором, который будет во-первых всегда статичным, а во-вторых - больше длины массива (иначе, цикл бы продолжался), поэтому Ваш код всегда будет давать андефайнд. Вместо иттератора лучше использовать this, когда человек кликнет на кнопку в this попадет текущая кнопка, по которой кликнули, и мы сначала удаляем активный класс у всех кнопок и контента, а потом текущей кнопке его назначаем. Второе, как я понимаю, что индекс кнопки и индекс таба должен совпадать, иначе код надо будет усложнить. Если индексы совпадают, то создаем локальную переменную, которая вернет индекс кнопки, по которой кликнули, просто находим индекс массива кнопок по объекту this, то есть по кнопке, на которой произошло событие, далее берем этот индекс и находим элемент по этому индексу из массива контента, и ему назначаем активный класс. В результате меньше функций.
Если индексы НЕ совпадают, тогда можно пройтись по всем табам и кнопкам и задать им дата атрибуты, а потом при клике вытаскивать по дата атрибутам нужные элементы и назначать классы, а вот удалять классы, думаю оптимально перед тем, как задать, и удалять классы у всех, а потом активным их задавать.

Comments

[N!crO]

var tabsWrapper = document.querySelector('.services .container');
var tabsBtn = tabsWrapper.querySelectorAll('.services__tabs-item');
var tabsContent = tabsWrapper.querySelectorAll('.services__item');

var activeBtnTab = tabsWrapper.querySelector('.services__tabs-item--active');
var activeContentTab = tabsWrapper.querySelector('.services__item--active');


tabsWrapper.addEventListener('click', function(event)  {

    event.preventDefault();
    var target = event.target;

    if (target.parentNode.classList.contains('services__tabs-item')) {

        for (var i = 0; i < tabsBtn.length; i++) {

            if (tabsBtn[i] === target.parentNode) {
                activeBtnTab.classList.remove('services__tabs-item--active');
                activeBtnTab = target.parentNode;
                activeBtnTab.classList.add('services__tabs-item--active');

                activeContentTab.classList.remove('services__item--active');
                activeContentTab = tabsContent[i];
                activeContentTab.classList.add('services__item--active');

                return;
            }

        }

    }

});