0

Задавала вопрос о том, что лучше использовать: простой код или модули при написании простого скрипта для страницы. На тостере ответили, что модули, но мой модуль ужасен.

О модулях больше всего понравилось описание тут.

Вопрос: что не так с моим модулем?

Пример для простого аккордеона:

"use strict";
$(document).ready(function(){
  if($('.accordion').length){
    accordion.init();
  }
});

var accordion = (function(){


  var _openContent = function(el){

    var
        wrap = el.closest('.accordion'),
        item = el.closest('.accordion__item'),
        items = wrap.find('.accordion__item'),
        content = item.find('.accordion__content'),
        contents = wrap.find('.accordion__content'),
        dur = 500;

    if(!item.hasClass('on')){
      items.siblings().removeClass('on');
      item.addClass('on');    

      contents.stop(true,true).slideUp(dur);
      content.stop(true,true).slideDown(dur);
    } else {
      item.removeClass('on');
      content.stop(true,true).slideUp(dur);
    }
  }

  // Инициализация  
  return {
    init: function(){

      $('.accordion__trigger').on('click', function(e){
        e.preventDefault();

        var
        $this = $(this);

        _openContent($this);
      });
    }
  }			
})();
*,
*:before,
*:after {
  box-sizing: border-box;
}

html,
body {
  margin: 0;
  padding: 0;
}

a {
  text-decoration: none;
  color: inherit;
  transition: all .27s ease-in-out;
}

a:hover {
  color: tomato;
}

ul {
  padding: 0;
  margin: 0;
  list-style-type: none;
}

body {
  font-family: 'Raleway', sans-serif;
  font-size: 1rem;
  line-height: 1.5rem;
  color: #727272;
}

.accordion {
  width: 100%;
  margin: 2rem auto;
  padding: 0 2rem;
}

.accordion__item {
  box-shadow: 0 2px 7px rgba(0,0,0,0.17);
}

.accordion__title {
  text-transform: uppercase;
  margin: 0;
}

.accordion__trigger {
  display: block;
  color: #fff;
  padding: .5rem 1rem;
  background: #9c27b0;
  box-shadow: 0 2px 7px rgba(0,0,0,0.27);  
  padding-left: 3.5rem;
  position: relative;
}

.accordion__trigger:before {
  content: '+';
  position: absolute;
  left: 0;
  top: 0;
  bottom: 0;
  width: 2rem;
  text-align: center;
  line-height: 2.5rem;
  background: #ff4051;
  font-weight: 800;
  transition: all .34s ease-in-out;
}

.on .accordion__trigger:before {
  content: '-';
}

.accordion__trigger:after {
  content: ' ';
  position: absolute;
  left: 2rem;
  top: 0;
  bottom: 0;
  width: 0;
  height: 0;
  border-top: 1.25rem solid transparent;
  border-bottom: 1.25rem solid transparent;
  border-left: .5rem solid #ff4051;
}

.accordion__trigger:hover {
  color: #fff;
  background: #7b1fa2;
}

.accordion__content {
  padding: 1.5rem 3.5rem;
  display: none;
}

.accordion__content span {
  display: block;
  text-transform: uppercase;
  color: #212121;
  font-weight: 700;
  margin-bottom: 1rem;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<div class="accordion">
  <ul class="accordion__list">
    <li class="accordion__item">
      <a href="#" class="accordion__trigger">
        <h5 class="accordion__title">Lorem ipsum 1.</h5>
      </a>
      
      <div class="accordion__content">
        <p>
          <span>Content 1. </span>
          Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem.
        </p>
      </div>
    </li>
    <li class="accordion__item">
      <a href="#" class="accordion__trigger">
        <h5 class="accordion__title">Lorem ipsum 2.</h5>
      </a>
      
      <div class="accordion__content">
        <p>
          <span>Content 2. </span>
          Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem.
        </p>
      </div>
    </li>
    <li class="accordion__item">
      <a href="#" class="accordion__trigger">
        <h5 class="accordion__title">Lorem ipsum 3.</h5>
      </a>
      
      <div class="accordion__content">
        <p>
          <span>Content 3. </span>
          Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem.
        </p>
      </div>
    </li>
  </ul>
</div>

15
  • Я бы не использовал слова типа "ужасен". Вполне годный код. А вот тостер - это ещё то гетто ИМХО.
    – user207618
    31 окт 2016 в 13:15
  • Оффтоп: стили сами придумали или где-то взяли?
    – user207618
    31 окт 2016 в 13:15
  • 1
    Весьма не нравится, пожалуй, одно - слишком много обращений к DOM. Помещение в модуль аккордеона - нормально, кто-то вообще ратует за то, что код до абсурдного максимально должен быть модульным. Но всё же надо знать меру. Тут можно.
    – user207618
    31 окт 2016 в 13:32
  • 1
    @Other, jsbin.com/wetiso/edit?css,output =))
    – SVE
    31 окт 2016 в 16:02
  • 1
    Супер! Мои благодарности Вам летят методами OSI! Очень неплохо, добавлю Вас в авторы таблицы по праву.
    – user207618
    2 ноя 2016 в 14:25

1 ответ 1

2

Пару небольших замечаний:

  1. Использовать один стиль для кавычек (либо одинарные, либо двойные). "use strict"; у вас в двойных, а остальных местах - одинарные
  2. Для ; тоже выбрать один стиль. Если ставите везде, то у вас пропущена после закрытия функции (Function Expression) var _openContent = function(el){ /* ... */ }; и тут return { /* ... */ };
  3. Вы используете в модуле $ - внешнюю библиотеку, её лучше передавать в сам модуль как параметр:

    var accordion = (function($){ /* ... */ })($);

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

  1. "use strict"; лучше перенести непосредственно в модуль, что бы он распространялся именно на него var accordion = (function($){ 'use strict'; /* ... */ })($);

  2. Думаю, было бы неплохо, если бы в модуль можно было передавать параметры (wrap, item, dur и т.д.) А значения, которые у вас есть сейчас, оставить по умолчанию. Выглядело бы это как то так:


// в init передаем объект с параметрами
accordion.init({
  dur: 1000
});
// ...
_openContent($this, setting); // передаем эти параметры в _openContent
// ...
var _openContent = function(el, setting){
  setting = setting || {}; // по умолчанию пустой объект
  var dur = setting.dur || 500; // -> 1000
  var wrapSelector = setting.wrapSelector ||'.accordion';  // -> '.accordion'
  1. Рекомендация (как и все остальные пункты), но тут уже дело вкуса. Если значение переменной является jQuery-объектом, то имя переменной можно начинать с $ - var $container = $('.container'). Тогда в коде сразу понятно (например, по вашему примеру, тут: contents.stop(true,true)), что мы работаем в данном месте с jQuery-объектом, а не с обычным js-объектом, у которого есть метод stop. В процессе написания кода, рефакторинга или поиска ошибок становится ясно, с чем работаем (возможно и не сразу, но я думаю вы поняли о чем я)

Для первых двух пунктов я рекомендую использовать eslint (или другие синтаксические анализаторы кода), которые сразу будут подчеркивать (выводить) такие ошибки.

И если ещё не сформировался свой стиль написания кода или когда работаете в команде, удобно использовать единый стиль, например Google JavaScript Style Guide или StandartJS (по которому не нужно ставить точку с запятой). Сам придерживаюсь Airbnb JavaScript Style Guide (на русском) - очень хорошо написан.

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими пользовательским соглашением, политикой конфиденциальности и политикой о куки

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.