Skip to content

Conversation

@skad0
Copy link

@skad0 skad0 commented Jul 1, 2017

postcss and selector-parser were updated.
allowTag option was added and behaviour with pseudoclass was fixed

@skad0 skad0 force-pushed the kriant.update-check-rules branch from 48a0a38 to e7c4632 Compare July 1, 2017 23:05
@skad0
Copy link
Author

skad0 commented Jul 1, 2017

@eGavr @dima117

@eGavr
Copy link

eGavr commented Jul 2, 2017

Мне в целом /ok, но я бы как минимум научил allowTag принимать массив, а еще возможно переименовал эту опцию в excludeTags, чтобы было консистентно с excludeClasses.

Есть еще один мажорный вариант:

переименовать excludeClasses в exclude и научить его принимать и классы (c точкой в начале) и тэги (без точки в начале)

@dima117 что думаешь?

Copy link

@eGavr eGavr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А еще у тебя там есть баг, о котором мы лично поговорили)

lib/index.js Outdated

var validator = new CssNaming(config._config.excludeClasses, addError);
var validator = new CssNaming({
excludes: config._config.excludeClasses, allowTag: config._config.allowTag
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

обращение к private полю, надо звать config.getConfig()

selector.eachClass(function(cssClass) {
hasTargetBlock |= _this._validateClass(cssClass, blockName, rule);
selector.walk(function(node) {
node.type === 'class' && (hasTargetBlock |= _this._validateClass(node, blockName, rule));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а какой смысл от приведения? почему не хранить в hasTargetBlock булевые значения?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

для случаев составного селектора как я понимаю:
.not-taget .target {}

@skad0
Copy link
Author

skad0 commented Jul 2, 2017

Я тут сильно переделал, добавил тестов.
Есть сомнения на счет того, должна ли эта штука вообще учитывать возможность nesting. Возможно нужно все-таки чтобы она проверяла только css классы и просто игнорировала существование тегов или чего бы то ни было еще?

@tadatuta @eGavr @dima117

@tadatuta
Copy link

tadatuta commented Jul 3, 2017

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

@tadatuta
Copy link

tadatuta commented Jul 6, 2017

@eGavr ping?

@eGavr
Copy link

eGavr commented Jul 7, 2017

Мы не вольем всё равно без /ok-а @dima117

Дима, посмотришь? мне в целом /ok

Copy link
Contributor

@dima117 dima117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не можем влить эти изменения, т.к. в плагин добавлена логика, не входящая в его зону ответственности + она реализована не до конца. Если нужно, чтобы проверка наличия класса блока в селекторе не выполнялась, лучше отключить её явно, а не добавлять логику, заменяющую ее.

Посмотрите PR про возможность выборочного отключения правил валидации

@skad0 skad0 closed this Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants