Правильно ли написан js?

Question

[BB 8]

На днях приступил к изучению BOM и DOM! Поверхностно знаю/слышал/видел функции JQUERY и мне стало интересно написать их на чистом JS. Правильно ли реализована фича: toggle из JQuery:

JSFID

window.onload = function() {
	var addCl = document.querySelectorAll(".header ul li");
		for (var i = 0; i < addCl.length; i++) {
			addCl[i].onclick = function() {
				for (var r = 0; r < addCl.length; r++) {
					if(addCl[r].classList.contains("onactive")) {
						addCl[r].classList.remove('onactive');
						this.classList.add("onactive");
					} else {
						this.classList.add("onactive");
					}
				}				
			}
		}
}

?? Критика приветствуется :)

Answer 1

[gleendo]

Array.ptototype.forEach.call(document.querySelectorAll(`.header ul li`), item => {
  item.addEventListener(`click`, event => {
    item.classList.toggle(`onactive`);
  }, false);
});

Array.ptototype.forEach.call(document.querySelectorAll(`.header ul li`), item => {
  item.addEventListener(`click`, event => {
    if (item.classList.contains(`onactive`)) {
      item.classList.remove(`onactive`);
    } else {
      item.classList.add(`onactive`);
    }
  }, false);
});

var elements = document.querySelectorAll(".header ul li"); // Собираем коллекцию элементов

for (var i = 0; i < elements.length; i++) { // Перебираем в цикле каждый элемент коллекции
  elements[i].addEventListener("click", function() {
    if (this.classList.contains("onactive")) { // Если {{ при клике }} элемент содержит класс
      this.classList.remove("onactive"); // То удаляем этот класс
    } else { // Иначе
      this.classList.add("onactive"); // Добавляем класс
    }
  }, false);
}

Демо

// Конкретно под ваш пример, лучше будет такая реализация.
var elements = document.querySelectorAll(".header ul li"); // Собираем коллекцию элементов

var selected = elements[0]; // Сохраняем выделенный элемент

for (var i = 0; i < elements.length; i++) { // Перебираем в цикле каждый элемент коллекции
  elements[i].addEventListener("click", function() {
    if (selected) { // Если есть выделенный элемент
      selected.classList.remove("onactive"); // То убираем у него выделение
    }
    
    selected = this; // Сохраняем текущий элемент на котором произошло событие
    
    this.classList.add("onactive"); // Добавляем выделение текущему элементу
  }, false);
}

Comments

[BB 8]

НЕ, ну до этого уровня мне еще далеко! Но как для новичка, это приемлемое решение или за такое можно убить? :D

[BB 8]

iamevg_: Понял, спасибо

Answer 2

[Alexander]

(function() {
	var addCl = document.querySelectorAll(".header ul li");
		for (var i = 0; i < addCl.length; i++) {
			addCl[i].onclick = function() {
          [].forEach.call(addCl, function(el) {
               el.classList.remove("onactive");
          });
	        this.classList.add('onactive');
			}
		}
})();

Answer 3

[Stalker_RED]

В документацию по classList вы, похоже, не заглядывали. Сложно не заметить, что там есть готовый метод toggle, который сократит ваш код вдвое.

Знаете что будет, когда какой-то другой скрипт сделает на эти-же элементы свой элемент.onclick = function()...? Будет работать только тот обработчик, который был назначен позже.
Лучше использовать addEventListener().
То-же самое и про window.onload.

Если позже будут добавлены новые элементы в ваше меню, то они не будут обрабатываться, так как список элементов вы создаете один раз при загрузке страницы. (.header ul li это же меню, да?)

Comments

[BB 8]

Заглядывал, но с toggle что-то не получилось написать на чистом JS

Answer 4

[Niriter Inc.]

(function() {
  var addCl = document.querySelectorAll(".header ul li");
  for (var i = 0; i < addCl.length; i++) {
    addCl[i].onclick = function() {
        [].forEach.call(addCl, function(el) {
             el.classList.remove("onactive");
        });
        this.classList.add('onactive');
    }
    }
})();