Skip to content
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

Support mod(m) and elemMod(em) without second argument #346

Merged
merged 1 commit into from
Sep 21, 2016

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented Sep 12, 2016

Fixed #274

Am I do it right?

//cc @veged @zxqfox

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage increased (+0.005%) to 97.038% when pulling 7a48f32 on issue-274__mods-wildcard into 49a3e10 on master.

block('a').mod('size', '*').mix()('sized');
```
Оба шаблона будут применены к узлам BEMJSON-а, у которых блок равен 'a' и
присутствует модификатор 'size' (со значением отличным от `undefined`).
Copy link
Member

Choose a reason for hiding this comment

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

не только undefined это отсутствие модификатора, а ещё и ''

Copy link
Contributor Author

@miripiruni miripiruni Sep 13, 2016

Choose a reason for hiding this comment

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

@veged

  1. null не берем в расчет?
  2. что скажешь про явное указание значения *? Его хотим обрабатывать? Я бы оставил его.

Copy link
Member

Choose a reason for hiding this comment

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

Мы обычно пишем false везде.

@veged
Copy link
Member

veged commented Sep 13, 2016

looks ok

@miripiruni miripiruni force-pushed the issue-274__mods-wildcard branch from 7a48f32 to 34a6223 Compare September 14, 2016 11:48
@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.005%) to 96.962% when pulling 34a6223 on issue-274__mods-wildcard into b2f14b7 on master.

{ block: 'a', mods: { size: '', theme: 'dark' } }
```

But templates will not applied for entities:
Copy link
Member

Choose a reason for hiding this comment

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

will not applied -> will not be applied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miripiruni miripiruni force-pushed the issue-274__mods-wildcard branch from 34a6223 to 288964c Compare September 14, 2016 14:47
@miripiruni
Copy link
Contributor Author

@tadatuta thank you!

Updated.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.005%) to 96.962% when pulling 288964c on issue-274__mods-wildcard into b2f14b7 on master.

Copy link
Member

@qfox qfox left a comment

Choose a reason for hiding this comment

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

Looks good at all. ;-)

@@ -123,6 +123,29 @@ The template are applied. Result:
<small class="item item_size_1"></small>
```

If second argument of `mod()` was omited or equal is to '*' then templates with any
Copy link
Member

Choose a reason for hiding this comment

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

equal is to → equal to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -123,6 +123,29 @@ The template are applied. Result:
<small class="item item_size_1"></small>
```

If second argument of `mod()` was omited or equal is to '*' then templates with any
value of modifier will be applied.
Copy link
Member

Choose a reason for hiding this comment

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

any value → any non-empty value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

block('a').mod('size').tag()('span');
block('a').mod('size', '*').mix()('sized');
```
Both templates will be applied to BEMJSON entities if block equals to 'a' and
Copy link
Member

@qfox qfox Sep 14, 2016

Choose a reason for hiding this comment

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

"BEMJSON entities" is a strange thing. Do we declare it somewhere?
BEM entity is absolutely another thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

block('a').mod('size', '*').mix()('sized');
```
Both templates will be applied to BEMJSON entities if block equals to 'a' and
'size' modifier exists (with value not `undefined` or `''`).
Copy link
Member

@qfox qfox Sep 14, 2016

Choose a reason for hiding this comment

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

equal neither to ... nor to ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{ block: 'a', mods: { size: '', theme: 'dark' } }
```

But templates will not be applied for entities:
Copy link
Member

Choose a reason for hiding this comment

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

Guess we should not use entities here. BEMJSON nodes or Nodes?

be applied for entities:be applied to:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -152,6 +176,10 @@ block('page').elem('content').elemMod('type', 'index').mix()({ block: 'mixed' })

`elemModVal` проверяются на соответствие после приведения к строке. Это поведение аналогично поведению проверки `modVal`.

Второй аргумент `elemMods()` также может отстуствовать или быть равен '*', в
Copy link
Member

Choose a reason for hiding this comment

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

'*' в бэктики?

@@ -152,6 +176,10 @@ block('page').elem('content').elemMod('type', 'index').mix()({ block: 'mixed' })

`elemModVal` проверяются на соответствие после приведения к строке. Это поведение аналогично поведению проверки `modVal`.

Второй аргумент `elemMods()` также может отстуствовать или быть равен '*', в
этом случае поведение аналогичное режиму `mods()` — шаблоны будут применены к
узлам с модификатором любого значения.
Copy link
Member

@qfox qfox Sep 14, 2016

Choose a reason for hiding this comment

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

почему здесь «узлам», а в англ. версии BEMJSON entities или entities? узлы лучше, и не противоречат key-concepts из методологии

@@ -117,12 +117,36 @@ block('item')
<small class="item item_size_1"></small>
```

Если второй аргумент `mod()` отсутствует либо равен '*' тогда к узлу будут
Copy link
Member

Choose a reason for hiding this comment

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

звезду в бэктики?

for (var i = 0; i < this.keys.length - 1; i++) {
val = val[this.keys[i]];
if (!val)
return false;
}

if (this.value === '*')
Copy link
Member

Choose a reason for hiding this comment

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

Технически, разницы с точки зрения шаблонизатора нет, но кажется, что лучше всё-таки использовать true, а не '*', потому что консистентность с i-bem onSetMod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onSetMod со значением модификатора true это же не то что проверка подпредиката «есть такой модификатор»… Я могу убрать волшебное слово wildcard, но, мне кажется, лучше оставить специальное значение '*'. Тогда можно будет явно писать mod('size', '*')

Copy link
Member

Choose a reason for hiding this comment

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

Ну черт с ним тогда, но имхо это неконсистентность на уровне onSetMod, и нет смысла делать так же, как и там.

Copy link
Member

Choose a reason for hiding this comment

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

Если же говорить про «явно писать mod('size', '*')» — таким образом у нас 2 способа добиться одного и того же. А зачем?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zxqfox убрал.

@@ -83,5 +83,33 @@ describe('Modes .mod(modName, modVal)', function() {
{ block: 'b', mods: { m: null } },
'<div class="b"></div>');
});

it('should support mod with wildcard', function() {
Copy link
Member

Choose a reason for hiding this comment

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

очень лукаво называть специальное значение строка со звездой вайлдкардом. Вайлдкард позволяет a*, *a, a*a, ??pup etc.

@qfox
Copy link
Member

qfox commented Sep 14, 2016

Github радует)

@miripiruni miripiruni force-pushed the issue-274__mods-wildcard branch 2 times, most recently from 9dd1d7b to 354e587 Compare September 19, 2016 13:56
@miripiruni
Copy link
Contributor Author

Updated

@miripiruni miripiruni force-pushed the issue-274__mods-wildcard branch from 354e587 to b32aab9 Compare September 21, 2016 12:22
@coveralls
Copy link

coveralls commented Sep 21, 2016

Coverage Status

Coverage increased (+0.007%) to 96.965% when pulling b32aab9 on issue-274__mods-wildcard into b2f14b7 on master.

@miripiruni miripiruni force-pushed the issue-274__mods-wildcard branch from b32aab9 to 5f5221f Compare September 21, 2016 12:36
@miripiruni miripiruni added this to the 7.3.0 milestone Sep 21, 2016
@coveralls
Copy link

coveralls commented Sep 21, 2016

Coverage Status

Coverage increased (+0.007%) to 96.965% when pulling 5f5221f on issue-274__mods-wildcard into b2f14b7 on master.

@miripiruni miripiruni merged commit 61e555b into master Sep 21, 2016
@miripiruni miripiruni deleted the issue-274__mods-wildcard branch September 21, 2016 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants