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.

Sign up for GitHub

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

Jump to bottom

BEMXJST: extend() mode fix #180 #363

Merged
merged 1 commit into from
Oct 20, 2016
Merged

BEMXJST: extend() mode fix #180 #363

merged 1 commit into from
Oct 20, 2016

Conversation

miripiruni
Copy link
Contributor

Fix #180

@coveralls
Copy link

coveralls commented Oct 4, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.201% when pulling f386271 on issue-180__extend into 5ab2e2f on master.

qfox
qfox reviewed Oct 4, 2016
View reviewed changes
lib/bemxjst/tree.js
@@ -488,6 +484,10 @@ Tree.prototype.prependContent = function prependContent() {
.match(new AddMatch('prependContent', this.refs));
};

Tree.prototype.wrap = function wrap() {
return this.def.apply(this, arguments).match(new WrapMatch(this.refs));
Copy link
Member

Choose a reason for hiding this comment

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

позиция этой декларации в файле разве что-то меняет?

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 меняет моё отношение к файлу. Порядок хочу. Чтобы wrap, extend, replace были рядом.

Copy link
Member

Choose a reason for hiding this comment

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

почему ты не хочешь отдельно делать стилические правки, а отдельно функциональные? религия?

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 потому что не вижу смысла в такой бюрократии.

Copy link
Member

Choose a reason for hiding this comment

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

Ты эту правку мог влить отдельно, поскольку она никак не влияет на бизнес логику, но «чтобы не разводить бюрократию» она теперь висит здесь и только путает. Это крайне нелогично.

tadatuta
tadatuta requested changes Oct 4, 2016
View reviewed changes
Copy link
Member

@tadatuta tadatuta left a comment

Choose a reason for hiding this comment

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

Please take a look at #180 (comment)

docs/en/5-templates-syntax.md
@@ -540,6 +541,33 @@ Result of BEMHTML templating:
<div class="wrap"><div class="quote">Docendo discimus</div></div>
```

#### extend

`extend` mode allows you extend current BEMJSON node. Notice what current
Copy link
Member

Choose a reason for hiding this comment

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

you to extend

what -> that

will be not -> will not be

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

@coveralls
Copy link

coveralls commented Oct 5, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.201% when pulling 772c1e6 on issue-180__extend into 5ab2e2f on master.

@miripiruni
Copy link
Contributor Author

miripiruni commented Oct 5, 2016 •
edited
Loading

@tadatuta can we fix extend() (to extend this) only? Why we need extendCtx()? What’s usecase? Can we use extend() like this? https://github.com/bem/bem-xjst/pull/363/files#diff-cfe236c3835ef2084ca4676d860a5fe2R56

@miripiruni
Copy link
Contributor Author

@tadatuta updated. Now extend() extends this.

@coveralls
Copy link

coveralls commented Oct 5, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.201% when pulling 8b10556 on issue-180__extend into 5ab2e2f on master.

@tadatuta
Copy link
Member

tadatuta commented Oct 5, 2016

Such command grep -lir "'ctx." *.blocks gives 4 results for my current project. No so many to widen API but idea was to keep it consistent with applyCtx.

@miripiruni
Copy link
Contributor Author

@tadatuta Why not https://github.com/bem/bem-xjst/pull/363/files#diff-cfe236c3835ef2084ca4676d860a5fe2R56 ?

@miripiruni
Copy link
Contributor Author

— check applyNext()
— how introduce extend() via applyNext(changes)

@miripiruni
Copy link
Contributor Author

— check several extend()

@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 96.999% when pulling 159c425 on issue-180__extend into 3f3169e on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 96.999% when pulling 159c425 on issue-180__extend into 3f3169e on master.

@miripiruni miripiruni mentioned this pull request Oct 19, 2016
Closed
@miripiruni miripiruni added this to the 8.2.1 milestone Oct 19, 2016
@miripiruni miripiruni merged commit ff8ca15 into master Oct 20, 2016
@miripiruni miripiruni deleted the issue-180__extend branch October 20, 2016 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qfox qfox qfox left review comments

@tadatuta tadatuta tadatuta approved these changes

Assignees
No one assigned
Labels
semver: patch
Projects
None yet
Milestone
8.3.1
Development

Successfully merging this pull request may close these issues.
4 participants
@miripiruni @coveralls @tadatuta @qfox