Не слишком ли жесткий костыль, как правильней записать такой jqyery код?

Question

[Юрий Янин]

tooglecloss = function(e) {
                $(".mobile-navigation").animate({
                "width" : "0%" 
            } , 1500 , function(){
                $(".mobile-navigation").css("display","none");
                $(".cmn-toggle-switch").removeClass("active").addClass('not_active');
            });
    };
    
    
    $('.not_active').on('click', function(e) {
        if($(this).hasClass('not_active')) {
                $(".mobile-navigation").animate({
                "width" : "40%" 
            } , 1500 ); 
                $(".cmn-toggle-switch").removeClass("not_active").addClass("active");
                $(".mobile-navigation").css("display", "block");
                $(".active").on('click', tooglecloss );
        }
        else {
            $(".active").off('click', tooglecloss );
        }
    });

Проблема была в том, что после добавления нового класса active, событие на него не привязывалось из-за уже сформированного Dom дерева, а при удалении not_active его собственное событие все равно выполнялось по той-же причине. В итоге я написал такой костыль с проверками классов. Но есть ощущение, что все намного проще должно быть. Кто укажет на ошибки?

Comments

[svarnoi420]

вам нужно открывать менюшку по клику и закрывать ее также?

[Юрий Янин]

svarnoi420: Да. Но кнопки две. Одна снаружи на сайте, другая в выплывающем меню. Данный скрипт уже работает, можете посмотреть структуру самой кнопки на сайте reedbuck.ru меня же больше интересует насколько плохо написан данный код. Все же хочу подтянуть свои навыки в правильности строения.

[svarnoi420]

Вижу. Во-первых, не нужно вещать событие на состояние кнопок. Во-вторых, выберите для себя метод как отделять классы js от стилизации, чтобы другой человек зашел и сразу увидел с чем работает js, да и себе на будущее. К примеру, я захочу переименовать класс на другой и все сломается. Один из вариантов добавлять префикс js-*. На нем не должны быть навешаны стили! Например, js-mobile-nav-toggle. И сразу вижу за что отвечает класс в js.

[Юрий Янин]

svarnoi420: это скорее методология построения верстки. Меня волнует именно сам скрипт. Не покидает ощущение, что всё это можно было уместить в 10 строк, а не в 20.

Answer 1

[svarnoi420]

Я бы написал так:
Пусть у внешней и внутренней кнопки будет один класс - js-mobile-nav-toggle. Состояния будет одно active, т.е. по умолчанию оно будет как бы не активным (not_active удаляем).

.mobile-navigation {
  position: fixed;
  left: -100%;
  top: 0;
  width: 380px;
  height: 100%;
  background-color: grey;
}

$('.js-mobile-nav-toggle').on('click', function (evt)  {
  var $mobNav = $('.mobile-navigation');
  var $this = this; // сохраняем текущий контекст. this == js-mobile-nav-toggle, т.е. тому, на что повещено событие
  if ( $mobNav.hasClass('active') ) {
    $movNav.animate({ "left":  0 } , 1500 , function () {
       $mobNav.removeClass('active');
       $($this).removeClass("active");
    });
  } else {
    $movNav.animate({ "left" : "-100%" } , 1500 , function () {
       $mobNav.addClass('active');
       $($this).addClass("active");
    });
  }
});

Comments

[Юрий Янин]

Тоесть в любом случае всё упирается в проверку наличия класса?

[svarnoi420]

Юрий Янин: код должен работать. Сейчас напишу подробности тогда с классом active.

[Юрий Янин]

svarnoi420: код не работает. Общий принцип пути понятен. Только вот мне кажется это еще более сложно написано чем писал я.

[svarnoi420]

я кешировал DOM объект. Зачем его вызывать три раза, когда можно один. Советую почитать про хорошие практики в jquery и в js в целом. В обще правильнее делать появление меню через позиционирование.

[Сергей delphinpro]

$($this) - лишнее. $this - у вас уже jquery-объект.

[svarnoi420]

Сергей: нет, в колбек передается обычный this.

[Сергей delphinpro]

svarnoi420: угу, как-то странно вы пишете...
var $this = $(this);
...
$this.removeClass("active");
так лучше, имхо

[svarnoi420]

Сергей: тут вы правы. Да и доллар в именование добавил, хотя this пока не являлся jquery объектом.

Answer 2

[Егор Неведов]

ну, not_active вообще не нужен.
Анимацию, если возможно, переписать на css.
Вместо добавлений - удалений класса
.toggleClass("active", state); //state = 0 / 1
Думаю меньше 10 строк будет.