menu: add a tmpl-spec for group with and w/o content #1543
Conversation
|
Фейл не мой: |
|
@narqo @dfilatov вы влили PR, в котором сломались gemini-тесты :( https://travis-ci.org/bem/bem-components/builds/66027954 Я не знаю как PR про checkbox сломал modal, но так и есть. |
|
Судя по diff-картинкам там изменилась высота окна. Это точно мы? #1544 |
|
Да, какая-то фигня с высотой произошла. Видимо, достаточно переснять скриншоты |
| content : [ | ||
| { | ||
| elem : 'group', | ||
| title : 'Group 1' |
There was a problem hiding this comment.
Зачем нам тесты на menu__group без контента? Ну да, оно как-то отрендерится шаблоном, но зачем?
Пример/тест menu__group есть тут, так важно иметь для него отдельный файлик?
There was a problem hiding this comment.
Вот ты спрашиваешь, а оно без того фикса падало совсем.
Про пример group — там написано theme disabled, ничего про групп не написано.
There was a problem hiding this comment.
@narqo я еще думал про контент с чем-то не menu-item: строкой, другими блоками...
There was a problem hiding this comment.
Вот ты спрашиваешь, а оно без того фикса падало совсем
Оно и не предполагалось (и не предполагается), что будет использовать без контента. Проверку на content добавили в качестве «хака» в процессе обсуждения. Я предлагаю относиться к этому не как к фиче, а как к «бекдору», про который знают только особо-любопытные и отчаянные. Никаких гарантий на уровне BEMJSON API и прочее.
Про пример group — там написано theme disabled, ничего про групп не написано
Можно просто файлик переименовать (с учетом сказанного выше, про «не предполагается»)? А то похоже на тесты ради бейджика в кавережде.
There was a problem hiding this comment.
я еще думал про контент с чем-то не menu-item: строкой, другими блоками.
Мы такое не поддерживаем (во всех смыслах фразы ;). В menu могут быть menu-item или __group. Все остальное — на свой страх и риск. Обсуждали
There was a problem hiding this comment.
Для наполнения пустого меню есть
menu#setContent()
Который, к слову, абсолютно не брезгует произвольным bemjson.
There was a problem hiding this comment.
Про пустую группу я выше написал: все, что сейчас нужно — доработать документацию и явно сформулировать, что пустых групп небывает.
Для наполнения пустого меню есть menu#setContent()
Который, к слову, абсолютно не брезгует произвольным bemjson
Во всей библиотеке, ни один блок не принимает bemjson — только HTML-строки или jQuery-коллекции (с правильно-подготовленным DOM, в соответсвии с документацией на блок), мы для этого jsdoc пишем. Запихнуть туда что угодно, в JS можно всегда — вопрос в том, что поддерживать такие кейсы, в долгосрочной перспективе, нас не хватит даже внутри Я.
There was a problem hiding this comment.
@narqo Да, действительно перепутал. Имел ввиду, что он примет произвольный html/dom конечно же.
Не нашёл упоминания в jsdoc ограничения на правильно-подготовленным DOM, в соответсвии с документацией на блок. Зато сам метод тестируется при этом строкой.
Не вижу проблемы с оформительными элементами внутри menu. Любой другой блок Сам блок menu абсолютно не переживает за то как именно в его dom лежат menu-item, главное только что внутри. На полное отсутсвие таковых он тоже не жалуется.
Прошу прощения за оффтоп. Кажется здесь была речь про тестирование шаблонов.
There was a problem hiding this comment.
Не вижу проблемы с оформительными элементами внутри menu.
Просто для общего описания проблемы: когда ты декларируешь, что некий menu-item можно завернуть «во что-то» неизвестное, это означает, что рано или поздно, к тебе придут пользователи, у который после такого оборачивания, внезапно перестали (в продакшне) работать селекты/меню/etc. и это, по мнению окружающих, теперь твой блокер. Поэтому, во всех случаях, касающихся расширения скоупа знаний блока, мы за то, чтобы этого не делать. Про это все, надеюсь, будет сформулировано в #1208.
Но вообще, да, мы отклонились от оригинального замечания: я считаю, что проверки на работоспособность menu__group в рамках 20-theme-disabled достаточно. Можно переименовать файл в какой-нибудь 20-theme-disabled-group. Никаких тестов на пустые группы добавлять не надо.
There was a problem hiding this comment.
Поэтому, во всех случаях, касающихся расширения скоупа знаний блока, мы за то, чтобы этого не делать. Про это все, надеюсь, будет сформулировано в #1208.
[спойлер]
Уже совсем скоро встречайте на ваших экранах обновление раздела принципов разработки, где все это будет подробно расписано. #1208 - следующий документ, который мы опубликуем чуть позже.
@zxqfox, расскажи что именно хочется? Сейчас ничего про отлов ошибок нет. |
|
@blond когда какие-то блоки не требуют использования каких-то параметров, как обязательных, стоит кидать эти ошибки. Но если их кидать, то хорошо бы как-то их проверять, что они правильно кидаются. Т.е. ловить каким-то образом |
|
@blond думаю, что тут важно отличать обычный вывод от брошенных ошибок. |
Да, понял. Как сделать это просто — не знаю :( |
|
@zxqfox rebase pls |
|
@sipayRT так вроде же не только ребейз, там еще не до конца решили что с пустой группой делать. |
|
решили же:
|
|
Вообще, по большому счету не очень важно, что там внутри, если можно переопределить шаблон (а это возможно). Но дока, код и тесты должны быть синхронизированы, на мой взгляд. Поэтому, тут надо прийти к одному знаменателю: сейчас даже тесты и код расходятся: я не вижу причин не проверить, что код не падает на пустой группе. Но это мое личное мнение ;) |
|
Обсудили с @narqo и @dfilatov. Решили, что:
Кроме того, нужно добавить в документацию о том, что поле |
|
@aristov чуваки, код все равно поддерживает пустые группы. зачем этот рассинхрон? |
|
@zxqfox это не рассинхрон:
|
|
@narqo nothing разве входит в something? отсутствие входит в наличие? |
|
Я ничего не понял, но, кажется, в контексте обсуждения — да. |
Fixes #1542
Ref #1523