Необходимо ли оптимизировать самописные табы на JQuery?

Question

[chupok]

Добрый вечер!

Написал небольшой скрипт для вкладок на JQuery. Все совершенно просто: в родительском блоке с классом about существуют три блока (article), каждый из которых становится видимым при нажатии на соответствующую кнопку (menu>li). По умолчанию открыт первый блок. Кстати, за видимость отвечает класс about-current, хотя можно было бы скрывать слои через hide().

JQuery начал изучать недавно и практикуюсь пока на велосипедах. Так вот вопрос: возможно ли оптимизировать, необходимо ли это в данном случае?

Спасибо!

<script>
	$(document).ready(function(){
		// Первый блок открыт по умолчанию
		$(".about article").eq(0).addClass("about-current");
			
		// По клику открываем соответствующий блок, остальные скрываем
		$(".about menu li").eq(0).click(function(){
			$(".about article").eq(0).addClass("about-current");
			$(".about article:not(:eq(0))").removeClass();
			return false;
		});
			
		$(".about menu li").eq(1).click(function(){
			$(".about article").eq(1).addClass("about-current");
			$(".about article:not(:eq(1))").removeClass();
			return false;
		});
			
		$(".about menu li").eq(2).click(function(){
			$(".about article").eq(2).addClass("about-current");
			$(".about article:not(:eq(2))").removeClass();
			return false;
		});
	});
</script>

Answer 1

[Евгений Петров]

1. Независимо от сложности приложения, которое вы пишете, нужно помнить, что операции поиска элементов очень дорогостоящие. Нашли один раз свои элементы (в данном случае по классу) и запомнили. А дальше уже с помощью eq(), filter() из них выбираете нужные. И неважно — простая или сложная структура в документе, сложные или не очень действия предусмотрены функционалом. Привычка — второе Я.

2. Искать лучше не по всему документу, а внутри некоего контейнера — так быстрее и правильнее. В данном случае неактуально.

3. Для функциональности лучше использовать data-атрибуты. А классы вы можете менять, как перчатки.

4. При использовании addEventListener писать в обработчике return false не имеет никакого смысла.

5. Вы написали 3 обработчика, хотя в реальности нужен один.

6. Вы сначала находите элементы, а потом навешиваете обработчик на событие click, а правильнее отслеживать событие в документе и обрабатывать его. К тому же, в этом случае не придётся ждать формирования DOM.

7. Ожидание формирования DOM можно делать через $() вместо $(document).ready().

Итого ваш скрипт (для простоты я не сохраняю результат поиска вкладок, поскольку не знаю остальных действий):

$(document).on('click', '.about menu li', function(e){
	$(this).addClass('about-current').siblings('about-current').removeClass('about-current');
});

$(function () {
	$(.about menu li).eq(0).trigger('click');
});

Comments

[chupok]

Благодарю. Логика понятна, буду исправлять скрипт.)

Answer 2

[andreyqin]

Вот вам хорошая реализация табов:

$('.wrapper-tabs').on('click', 'span:not(.active)', function () {
    $(this)
        .addClass('active')
        .siblings()
        .removeClass('active')
        .closest('.wrapper-tabs')
        .find('.tab-box')
        .eq($(this).index())
        .show()
        .siblings('.tab-box')
        .hide();
});

А это схематичная разметка:

<div class="wrapper-tabs">
    <span class="active">Tab 1</span>
    <span>Tab 1</span>
    <div class="tab-box">Content Tab 1</div>
    <div class="tab-box">Content Tab 2</div>
</div>

Изучайте.

Comments

[Евгений Петров]

@andreyqin

1. Построена на тэгах и классах. Плохо
2. Семантика разметки низкая. Плохо.
3. Навешиваем обработчик после поиска контейнера. Средне.
4. Каждый раз при клике заново ищем элементы, причём по всем потомкам. Очень плохо.

Ну и как можно сказать, что этот код — «хорошая реализация табов»?

[andreyqin]

1. Почему на классах плохо?
2. Данная разметка просто пример.
3. Если я вас правильно понял, то ответ будет такой - это обычное делегирование события. У вас есть варианты сделать лучше? Предлагайте.
4. Тоже не совсем понятно что вы имеете в виду.

[Евгений Петров]

@andreyqin

1. Почему на классах плохо?

Потому, что визуализация и функционал должны быть максимально отделены друг от друга. Какой смысл затевать возню с паттернами проектирования, обсуждать MVC, MVP и прочие штуки-дрюки, а потом в скриптах базироваться на визуальных атрибутах?

Даже в противостоянии микроданных и микроформатов победили первые.

2. Данная разметка просто пример.

Одно дело — писать пример на основе существующей вёрстки, другое — написать свою, а по итогу выясняется, что она не лучше исходной. Не принимайте так лично мои замечания — я комментирую код. А предложенная структура несемантична, нелогична и непрактична.

3. Если я вас правильно понял, то ответ будет такой - это обычное делегирование события. У вас есть варианты сделать лучше? Предлагайте.

Обычное делегирование — это ловить в глобальном объекте всплывшее событие (из-за IE в document). Сначала найти элемент, а потом на его уровне ловить всплывающее событие — а в чём выигрыш?

4. Тоже не совсем понятно что вы имеете в виду.

При каждом клике обыскивать всех(!) потомков в поисках класса — это очень плохо. А если потомков в содержании, которое переключают табы, будет несколько сотен элементов? Траверс — крайне трудоёмкая операция, а вы её проделываете при каждом клике в полном объёме.

[andreyqin]

1. На самом деле обсуждение этого вопроса - чистой воды холивар. Тут нет "хорошо" или "плохо", при правильном использовании и именовании никакие проблем возникать не будут. Много у вас будет на странице элементов с классами wrapper-tabs или tab-box?

2. А в чем, если не секрет, её несемантичность, нелогичность и непрактичность? Ну, вместо span можно использовать button, раз это кнопки. Что еще?

3. Повышается производительность за счет того, что событие навешивается только на один родительский элемент.

4. А как еще вы будете искать неактивные кнопки переключения?

[Евгений Петров]

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