MVC

[alextsk]

1 разбиваем компонент на 3 файла - модель вью и контроллер. в четвертом файле их импортируем и собираем 2 разграничиваем ответственность: вью не знает про модель, модель про вью, их связывает контроллер. никто кроме вью не знает про DOM. никто, кроме модели ничего не считает 3 чистим код вью, не изменяемые стили( позишн абсолют, константы) могут отлично жить в css чтобы не замусоривать код

[alexanderkif]

вью не знает про модель, модель про вью, никто, кроме модели ничего не считает

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

[alextsk]

только пикселы считаются? у слайдера же есть значение, оно где вычисляется?

методы модели должны относиться к самой модели

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

[alexanderkif]

привет.

ну да, вью высчитывает только отображение. там все методы draw для рисования конкретного элемента на основании модели: пределов мин-макс, значений 1 и 2, интервалов, вертикального вида и т.п. вью не знает ни о контроллере, ни о модели. модель ему отдает метод контроллера toDraw() в поле model и он же инициирует перерисовку (event draw).

модель вообще ни о ком не знает, она может только устанавливать свои значения и проверять их.

контроллер обо всех знает и обрабатывает действия юзера. ивенты отдают ему свое положение, а он вычисляет активную точку (sliderm3MouseDownListener(e)) и транслирует пикселы в значения модели (changeRange(e)), обновляет модель и командует вью перерисовать.

вроде ж все правильно. контроллер и должен контролировать когда и кому листнер воткнуть, а кому выткнуть.

[alextsk]

контроллер и должен контролировать когда и кому листнер воткнуть,

не должен, это листенеры браузера, ты можешь завести свои события, и тогда да

обрабатывает действия юзера

совершенно точно не контроллер

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

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

кстати, спрашивал сергея, он предлагает конфиг в еще одну отдельную модель вынести.

[alexanderkif]

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

вот вообще не врубаюсь в эти слова. пожалуйста объясни, чтоб я понял (я буду стараться понять)

[alextsk]

посмотри для примера игру жизнь у меня https://github.com/alextsk/conway-game-of-life/tree/refactoring/src, там тайпскрип, но в целом можно понять логику. если так же не понятно будет завтра в телеграм пиши - попробую подробнее на словах

[alexanderkif]

привет. три пункта в начале ишью сделал. проверь плз, правильно ли я все понял на этот раз )

[alextsk]

уже лучше, да. 1 нейминг - много повторов slider3m, зачем, мы знаем что мы внутри компонента, как в именах файлов так и в коде sliderm3MouseMoveListener - только усложняет чтение. 2 event observer - норм вариант только почему он во вью? модель тоже может им пользоваться. 3 файле не должно быть больше одного класса, если в файле один класс и он экспортируется - имя файла должно совпадать с именем класса, с большой буквы. 4 от dataset можно избавиться например так Object.assign(this, dataset) 6 .bind(this), 66); - почему 66? это магические числа, лучше их выносить в константу 7 value1 value2 можно более осмысленно назвать 8 селектор лучше определять уже в в вызывающем коде, компонент должен получать уже dom элемент, либо строку с селектором в параметрах, либо и то и то и уметь их отличать) 9 this.view.div.dispatchEvent(new Event('draw')); - почему тут браузерный event? у тебя же есть свои события 10 this.div - а вдруг там span?

countNewValue - получается ты отсылаешь в модель новое значение? это не задача вью, можно отослать новую позицию, еще какие-то данные полученные от пользователя, но новое значение модель считает сама.

[alexanderkif]

все сделал.

[alextsk]

демо тоже часть кода, вот такие штуки this.inputSlidermStepDiv = $('.sliderm-step', this.sliderm3pageInputDiv)[0]; тоже нужно рефакторить

посмотри на Object.assign в контроллере, есть подозрение что один там лишний, а другой нарушает инкапсуляцию

toDraw вызывает сам контроллер а должен реагировать на сообщение от модели представь например что this.model.setEndRange(model.endRange); это асинхронный запрос на запись в БД, тогда this.toDraw(); нарисует старое состояние

lineDiv - зачем нам знать что это див? this.scaleDiv = document.createElement("ul"); - тем более если он не див

this.lineDiv.className = "sliderm3__line"; - например у меня есть верстка со своими именами, я хочу чтоб слайдер у меня назывался my-awesome-slider-1234, c текущей реализацией это возможно?

TIME_PERIOD = 50; - не особо прояснилось его назначение

this.element.addEventListener('draw', this.draw); - где то еще емитится этот ивент?

        if (this.lineDiv) this.lineDiv.remove();
        this.drawLine();
        if (this.model.scale) this.drawScale();
        this.drawRange();
        if (this.model.interval) this.drawStartPoint();
        this.drawEndPoint();

непонятно, ты хотел чтобы drawLine drawRange drawEndPoint вызывались всегда?

if (activePoint == 1) сейчас контроллер проверяет что именно произошло, может лучше вью будет сразу слать именованные события? типа broadcast('changedFromField', ...)

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

[alexanderkif]

переписал. перепроверил.

[alextsk]

[].forEach.call(document.getElementsByClassName(dataJson.sliderClass), element => new Slider(element)); это должно быть в вызывающем коде, в этом случае в демо. в самом компоненте не должно быть ничего привязывающего его к данным.

    if(data.description == 'changedStartRange') {
     if(data.description == 'changedEndRange') {

в любом случае одно и то же происходит? зачем тогда это в контроллер выносить

element.classList.add("sliderm3"); - если передать базовый класс можно привязаться к нему $('${baseClass}__bem-element--bem-modyfier')

var model = new Model(element.dataset) - выглядит не очень. может лучше ввести дополнительный класс config который будет заниматься валидацией данных и хранить дефолтные значения для отсутствующих, чтобы не приходилось в каждый слайдер все параметры передавать

PERIOD_BETWEEN_REFRESH_WHEN_RESIZING - думаю THROTTLE_THRESHOLD будет норм)

refreshView - я вижу только в демо, т.е. если демо отключить слайдер перестанет работать?

    refreshView() {
        this.broadcast({refreshModel: this.model});
        this.refreshSlider();
    }

это вообще не ответственность вью, приказывать обновить модель. вью сообщает что произошло событие в интерфейсе и отрисовывает изменения когда получает новые данные. старайся исходить из принципа некомандных собщений, т.е. никто никому не приказывает, слои только уведомляют об изменениях в своем состоянии. и о чужом состоянии не знают, пока не получат сообщение. this.refreshSlider() - должен срабатывать в ответ на сообщение от модели и на основе приложенных к нему новых данных, а сейчас получается вью приказала модели чтото пересчитать и не дожидаясь ответа сама рисует. if (data.refreshModel) Object.assign(this.model, data.refreshModel); получается модель в этой схеме вообще не участвует?

[alexanderkif]

поправил.

[alextsk]

setModelFromEvent - получается модель меняется напрямую? все изменения в модели должны быть через события.

new Validate(dataset); - очень неочевидно что датасет может измениться. и почему valildate? в нем смесь дефолтных значений и нормализации - валидатор обычно просто определяет правильные ли данные. кстати, в es6 есть деструктуризация и дефолтные значения

element.classList.add("sliderm3"); - договорились css не трогать, а базовый класс все-таки передавать параметром

delete model.observers; - почему это так?

https://github.com/alexanderkif/modifiable-slider/blob/9931feb24b2abdaa99895d5f9f5bb79de01d7d72/src/View.js#L63 https://github.com/alexanderkif/modifiable-slider/blob/9931feb24b2abdaa99895d5f9f5bb79de01d7d72/src/View.js#L169 это не к mvc - но нужно такие длинные линии причесать - очень сложно читать. и что такое +5?

countRangeElement что делает метод? из названия я не понял https://github.com/alexanderkif/modifiable-slider/blob/9931feb24b2abdaa99895d5f9f5bb79de01d7d72/src/View.js#L202-L203 что хранит long? а startline? очень запутанный метод

[alexanderkif]

имена, длинные строки поправил. в конструктор слайдера теперь передаю имя класса.

[alextsk]

new Config(model); - тут не только в названии дело было когда ты используешь new, всегда создается новый объект, и тут он тоже создается, просто ты его игнорируешь и пользуешься побочным эффектом того что структура передается в конструктор по ссылке и неявно меняется. это неочевидно. это может выглядеть так let normalizedModel = new Config(model)

this.model = this.element.dataset; - это зачем? из-за setModelFromEvent? если да, то дело не в названии, а в том что, если я правильно понял, ты меняешь локальное представление напрямую, без запроса в модель. т.е. управление должно быть по схеме: двинул мышь -> запросил модель посчитать новое значение на основании координат -> получил от модели новое значение -> отрисовал сейчас вроде не так? сложно понять что происходит

[alextsk]

broadcastChangedPoint - почему передает все данные? они все изменились?

https://github.com/alexanderkif/modifiable-slider/blob/7f634c2e96067457032bbe6ef38b9bf540e2cef1/src/View.js#L150 draw line не только рисует линию но и устанавливает обработчик?

refreshSlider() - по прежнему плохо читается, ифам нужны {}(вообще всем ифам в проекте), а все безусловные вызовы сгруппировать. кстати, он все еще нужен тут?

countRangeElementBandAndIndent - слишком много всего делает, даже судя по названию он выполняет две задачи, а на деле он не только count но и draw. один метод должен выполнять одну задачу, надо это разносить.

проверь и другие методы на многозначность, если draw - то он должен только рисовать, count - считать, и для count- уточнить не должны ли эти рассчеты быть в модели.

[alextsk]

я пока не понимаю, почему их не получается разделить, это же разные действия band(Width? Position?)= countBand() indent= countIndent() ....... renderSomethingt(band, indent, isVertical)

сейчас если прочитать эти строки https://github.com/alexanderkif/modifiable-slider/blob/12d02592610b8e59e5b8c64e59659d31122dccdf/src/View.js#L209-L211

ничто нам не говорит, что будет отрисовка, я бы понял что произошел count, возможно записал в локальное состояние

и дальше там все смешано https://github.com/alexanderkif/modifiable-slider/blob/12d02592610b8e59e5b8c64e59659d31122dccdf/src/View.js#L213-L216 создание, классы, опять рассчет который на самом деле рисует стили, плюс еще стили. это нужно разделять по ответственностям

[alextsk]

{dataset.pointSize * 3 / 6 == .5, можешь объяснить в коде почему тут размер точки делится на два? причем таким странным способом

[alexanderkif]

сложно вспомнить уже, но скорее всего изначально размер шрифта шкалы устанавливался в одну шестую размера точки. потом показалось мало и дробь начала увеличиваться, пока не стала 3/6. локально исправил на /2. отдельно коммитить не буду. попадет на github в следующем текущем коммите.

[alextsk]

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

[alexanderkif]

сделал