-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ns.modelCollection не поддерживает разреженные данные #161
Conversation
Поговорили голосом. Роман решил сделать заход на поддержку разреженных коллекций |
Краткое содержание изменений:
// было
[ m1, m2 ]
insert( [m4, m5], 3 );
// стало
[ m1, m2, undefined, m4, m5 ]
// важно: insert вставляет:
insert(m3, 2);
[ m1, m2, m3, undefined, m4, m5 ]
// Чтобы установать элемент (с заменой) нужно использовать setModels()
// было
[ m1, m2 ]
setModels( [m4, m5], 3 );
// стало
[ m1, m2, undefined, m4, m5 ]
// и главное:
setModels(m3, 2);
[ m1, m2, m3, m4, m5 ]
|
А ещё может переименовать для единообразия: |
|
Есть замечания по мелочам, но о них можно поговорить позже. |
Практическая задача может быть такая например:
|
Итоговый changelog такой:
|
model.eventListeners[this.key] = this; | ||
|
||
// если ранее мы не переопределяли прототипный тригер | ||
// то переопеделим его | ||
// XXX это очень жёсткий bad practice... Лучше модэли элементы коллекции тогда наследовать от какого-то ns.Model.CollectionItem, где всё это прописать. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему bad practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Потому что модель могла уже как-то переопределить этот метод и здесь это переопределение может затереться.
Самое лучше — сделать базовый тип ns.ModelCollectionItem
где и переопределить trigger
.
И добавить проверку, что подмодель наследуется от ns.ModelCollectionItem
.
Но и это плоховато, потому что у нас получается standalone модель и модель, когда она становится подмоделью коллекции себя по-разному ведут.
С названиями методов в корне не согласен. Получается: Нужно хорошо проработать API, иначе все будут вечно путаться. |
|
} else { | ||
itemViewId = split.view_id; | ||
itemParams = no.extend({}, params, model.params); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот блок кода - корявый костыль, который ещё и дублируется.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это правда, но пока мне показалось легче оставить так. Могу поправить.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, там сам метод вообще больше похож на костыль (изначальный метод).
Он в 200 строк и это реальная каша, которую надо всю переписывать.
Я старался поменьше трогать существующий код.
Ром, ещё раз, почему ты не хочешь создать пустые модели и по ним отрендерить вьюшки? Мы так делаем и всё отлично работает. Ты вместо того, чтобы использовать практику, которая уже где-то используется, искривляешь API и усложняешь логику работы. |
Ох, многовато комментов, сложно отвечать.
Тут ничего не понял.
Что в этом плохо - не понимаю. |
Есть 2 варианта: делать фейковые модели и не делать их. Где я искривляю API я не понял. |
Тем, что не нужно обрабатывать её отсутствие. Логика будет проще Вообще, все остальные замечания сейчас не важны, потомучто я убеждён, что эту задачу нужно решать только на уровне модели, и не дальше. |
А чем логика пустой модели проще логики фейковой view? |
Проще тем, что viewCollection ничего не знает о том, что это какая-то особенная ситуация. Он просто работает со списком каких-то моделей. |
Это так да. А с фейковыми вью не нужна обёртка, коллекция сама вставляет пустое вью на место дырки в данных. |
Вид должен содержать только логику рисования моделей. |
С фейковыми моделям есть одна засада: они совсем фейковые. У твоего варианта (чтобы пустые модели создавала/заменяла ns.ModelCollection) есть конечно плюс — не надо усложнять viewCollection. |
И тут я понял, что ты, скорее всего, прав ) ... |
Поговорили голосом:
Ещё вот надо подумать про:
|
тут бы ещё вот о чём подумать: м.б. добавить модели статус empty? А то {empty: true} - это могут быть реальные данные... |
а на такой статус можно завязаться во viewCollection, и генерить такие декларации, на которые можно было бы матчиться в стандартном шаблоне. |
Conflicts: src/ns.modelCollection.js
В общем: переделал у себя на текущую версию |
Больше всего мне не нравится в текущей версии, что:
|
Вот такой вот пример:
Приведёт к такому массиву элементов:
Ожидаю:
Это by design?
/cc @edoroshenko @doochik