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

apply(modeName) must return values from this.ctx if no other templates #255

Merged
merged 1 commit into from
Jun 27, 2016

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented Apr 27, 2016

Fix for #236

Benchmarks test results within the error: (+/– 1% in different bundles).

@miripiruni
Copy link
Contributor Author

Updated!

if (ctxJS !== true)
js = utils.extend(ctxJS, js);
} else if (ctxJS === true) {
js = {};
} else {
js = ctxJS;
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Because of this.js.exec(context) now returns this.ctx.js by default.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@qfox
Copy link
Member

qfox commented May 11, 2016

I'd put totalMix into separate commit, all the rest is LGTM

@miripiruni
Copy link
Contributor Author

@zxqfox separate commit or PR?

@qfox
Copy link
Member

qfox commented May 11, 2016

:-D Commit will be enough to me.

@miripiruni
Copy link
Contributor Author

@zxqfox actually, totalMix is not needed anymore. See my comment above.

@miripiruni
Copy link
Contributor Author

May I merge?

@miripiruni miripiruni added this to the 6.5.2 milestone May 20, 2016
@@ -18,8 +18,8 @@ function Entity(bemxjst, block, elem, templates) {
this.canFlush = true;

// "Fast modes"
this.def = new Match(this);
this.content = new Match(this);
this.def = new Match(this, 'def');
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 it looks like not necessary, you’re right.

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 updated.

@miripiruni
Copy link
Contributor Author

ping

this.cls = new Match(this);
this.tag = new Match(this, 'tag');
this.attrs = new Match(this, 'attrs');
this.mod = new Match(this, 'mod');
Copy link
Member

Choose a reason for hiding this comment

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

Didn't noticed before, what the purpose of this? There is no mod property in bemjson as well as mod with template body. We have block('a').mod('m', 'v')(body) but not block('a').mod()(body).

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 agree. Thank you. 'mod' removed.

@qfox
Copy link
Member

qfox commented May 23, 2016

Not sure what the mod. LGTM at all.

@miripiruni
Copy link
Contributor Author

OK.
tumblr_mddkdtuc1a1r9qnhbo1_400

@miripiruni miripiruni merged commit 4c2433a into master Jun 27, 2016
@miripiruni miripiruni deleted the issue236-apply branch June 27, 2016 11:21
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.

None yet

2 participants