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

Docs: write about template arguments and arrow function (fix for #367) #370

Merged
merged 1 commit into from
Nov 17, 2016

Conversation

miripiruni
Copy link
Contributor

@tadatuta @zxqfox давайте согласуем сначала русскоязычную версию, а затем я добавлю перевод на английский, OK? Сейчас эта информация совсем не раскрыта в доке. Я решил срочно дополнить.

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage remained the same at 97.267% when pulling 71fd709 on issue-367__about-functions into a4e1a1c on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.267% when pulling 71fd709 on issue-367__about-functions into a4e1a1c 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.

У меня пара вопросов:

  • Достаточно ли этого описания для понимания что там передается? (кажется, что да)
  • Что можно убрать, чтобы не отвлекать от второстепенного?

# Функции шаблонов

Если тело шаблона является функцией, то вы можете использовать два параметра:
1. контекст выполнения шаблона
Copy link
Member

Choose a reason for hiding this comment

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

не хватает ;


Если тело шаблона является функцией, то вы можете использовать два параметра:
1. контекст выполнения шаблона
2. текущий узел BEMJSON, на который сматчился шаблон.
Copy link
Member

Choose a reason for hiding this comment

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

/vikka-mode on
В случае, когда тело шаблона является функцией, она вызывается с двумя аргументами:

  1. контекст выполнения шаблона;
  2. узел BEMJSON, удовлетворяющий подпредикату match (или как он там называется по научному).
    /vikka-mode off

* @param {Object} json
*/
attrs()(function(context, json) {
return …
Copy link
Member

Choose a reason for hiding this comment

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

лучше что-то конкретное {href: json.url}

* @param {Object} context
* @param {Object} json
*/
attrs()(function(context, json) {
Copy link
Member

@qfox qfox Oct 6, 2016

Choose a reason for hiding this comment

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

Последний раз, когда мы обсуждали, мы пришли не к ctx, json, а к node, ctx, потому что во всех текущих шаблонах такое: function() { var ctx = this.ctx; и this меняется на node, а ctx остается как есть.

Можно еще раз подумать

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

Copy link
Member

Choose a reason for hiding this comment

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

Я по-прежнему за node и ctx.
Во-первых, потому что this.ctx никуда не девается и будет очень странно, если название будет разное.
Во-вторых, если this.ctx — это сокращение от «контекст», то очень странно, если первый аргумент тоже будет context.
В-третьих, BEMJSON — это вообще ни разу не JSON и потому название json только больше будет сбивать с толку.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я подумал, что нет особого смысла в этом споре. Ведь каждый разработчик сам волен именовать эти аргументы. Главное, чтобы все улавливали суть. Пусть в доке будет node, ctx. Вальсируем дальше =)

);
```

Такие же параметры доступны в подпредикате `match()`.
Copy link
Member

@qfox qfox Oct 6, 2016

Choose a reason for hiding this comment

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

Тело подпредиката match() так же вызывается с этими параметрами

писать везде в таком стиле:

```js
match((ctx, json) => { return !context.mods.disabled && json.target; })
Copy link
Member

Choose a reason for hiding this comment

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

{} не нужны, достаточно: (node, ctx) => (!node.mods.disabled && ctx.target)


```js
match((ctx, json) => { return !context.mods.disabled && json.target; })
attrs()((ctx, json) => { … })
Copy link
Member

@qfox qfox Oct 6, 2016

Choose a reason for hiding this comment

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

addAttrs()((node, ctx) => ({href: ctx.url}))

block('link')(
/**
* @param {Object} context
* @param {Object} json
Copy link
Member

Choose a reason for hiding this comment

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

Кажется, этот кусок jsdoc ничего не дает ;-(

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
Copy link
Contributor Author

miripiruni commented Oct 6, 2016

Достаточно ли этого описания для понимания что там передается? (кажется, что да)

Сам же и ответил. Молодец =)

Что можно убрать, чтобы не отвлекать от второстепенного?

Я специально добавил эту информацию после всего основного рассказа «что доступно в теле шаблона». Убирать не нужно, нужно пояснять на примерах, что я и сделал.

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage remained the same at 96.999% when pulling ddc2612 on issue-367__about-functions into e3f2ade on master.

@tadatuta
Copy link
Member

tadatuta commented Nov 9, 2016

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

@miripiruni miripiruni force-pushed the issue-367__about-functions branch from ddc2612 to 8551260 Compare November 17, 2016 11:02
@miripiruni
Copy link
Contributor Author

Updated

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.999% when pulling 8551260 on issue-367__about-functions into 7efbc32 on master.

@tadatuta
Copy link
Member

LGTM 👍

@miripiruni miripiruni merged commit 706dd3f into master Nov 17, 2016
@miripiruni miripiruni deleted the issue-367__about-functions branch November 17, 2016 12:15
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.

4 participants