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

BEMHTML: Support nested mixes #280

Closed
wants to merge 3 commits into from
Closed

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented May 23, 2016

Fix #241

This PR about only support in mix something like this:

{
    block: 'b',
    mix: [
        { block: 'a', mix: { block: 'abc' } },
        { block: 'test', mix: [ { block: 'e', js: true }, 'some-block-name', 'another-block' ] }
    ]
}

out += nested.out;
}

// Process nested mixes from Templates
Copy link
Member

Choose a reason for hiding this comment

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

This function grows so much. I'm afraid it refused to be optimized in v8.

@miripiruni
Copy link
Contributor Author

@miripiruni
Copy link
Contributor Author

miripiruni commented Aug 3, 2016

  1. Build visited key with mods/elemMods (need sort) and cache it inside entity.
  2. Collect out as Object (to avoid classes duplication) and then serialize.
  3. Add tests.

@miripiruni miripiruni force-pushed the issue-241__nested-mixes branch 4 times, most recently from 5a89d99 to e1a3904 Compare August 11, 2016 16:55
@miripiruni
Copy link
Contributor Author

Updated

@miripiruni
Copy link
Contributor Author

@veged @zxqfox review needed.

@miripiruni miripiruni force-pushed the issue-241__nested-mixes branch 3 times, most recently from 49ea8e1 to c938dab Compare August 18, 2016 12:34
mix: {
block: 'b3',
mods: { test: 'opa' },
js: { data: '123' },
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 это ошибка, надо добавлять. Сейчас поправлю. Спасибо, что нашел!

@miripiruni
Copy link
Contributor Author

Встречались с @arikon @zxqfox обсуждали JS-параметры.

Почему у примиксованного блока выводятся JS-параметры если они написаны через BEMJSON непосредственно в mix, но задать их через шаблон по моде JS нельзя.

Решили, что нужно уметь задавать JS-параметры и через BEMJSON и через шаблоны.

@@ -238,4 +238,48 @@ describe('BEMJSON mix', function() {
'b__e_modname_modval"></div></div>');
});
});

describe('should support nested mixes', function() {
Copy link
Member

Choose a reason for hiding this comment

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

describe('nested mixes', function() {

it('should support nested mix', function() {

it('should support nested mix with js params', function() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tadatuta fixed!

@coveralls
Copy link

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.04%) to 96.886% when pulling d3d49d4 on issue-241__nested-mixes into d53646f on master.

@miripiruni miripiruni force-pushed the issue-241__nested-mixes branch 2 times, most recently from 2df3a84 to 9cd0e27 Compare September 5, 2016 15:16
@miripiruni
Copy link
Contributor Author

Removed commit about #147 bacause I try to implement apply all modes from nested mix.

As long as it can benchmark penalty I decided to move it in another PR.

@coveralls
Copy link

coveralls commented Sep 5, 2016

Coverage Status

Coverage increased (+0.01%) to 96.856% when pulling 9cd0e27 on issue-241__nested-mixes into d53646f on master.

@miripiruni
Copy link
Contributor Author

Description updated.

@coveralls
Copy link

coveralls commented Sep 5, 2016

Coverage Status

Coverage increased (+0.01%) to 96.856% when pulling 9cd0e27 on issue-241__nested-mixes into d53646f on master.

@miripiruni
Copy link
Contributor Author

Looks like we need refactoring and optimize renderMix function:

nested-mixes-with-escape-content-option

d536 — current HEAD
9cd0 — nested mix branch.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.059% when pulling 20b8a29 on issue-241__nested-mixes into 5ab2e2f on master.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.063% when pulling 29233cf on issue-241__nested-mixes into 5ab2e2f on master.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.063% when pulling 788c06c on issue-241__nested-mixes into 5ab2e2f on master.

@miripiruni
Copy link
Contributor Author

Test started…
{
  rev1: '29233cf9a03e32e06306a93b477381016559950c',
  rev2: '5ab2e2fc3d13ef95ae6c1fdd34f3cfe9f25580db',
  bemjson: 10000,
  dataPath: '/Users/miripiruni/bemjson',
  templatePath: './node_modules/web-data/templates.js',
}
Wed Oct 05 2016 17:50:49 GMT+0300 (MSK)
Total test time:  243405.277718
./lib/compare.py ./dat-29233-5ab2e ./dat-29233-5ab2e/29233cf9a03e32e06306a93b477381016559950c-10000-1475679173826.dat ./dat-29233-5ab2e/5ab2e2fc3d13ef95ae6c1fdd34f3cfe9f25580db-10000-1475679293051.dat
Percentile:  0.5
{ rev1: 8.396458,
  rev2: 7.888352,
  'diff abs': 0.5081060000000006,
  'diff percent': 6.05143263981075 }

Percentile:  0.9
{ rev1: 11.214023,
  rev2: 10.546001,
  'diff abs': 0.6680219999999988,
  'diff percent': 5.957023630145919 }

Percentile:  0.95
{ rev1: 12.401017,
  rev2: 11.752831,
  'diff abs': 0.648185999999999,
  'diff percent': 5.226877763331828 }

screen shot 2016-10-05 at 17 55 37

addInit: addJSInitClass
},
null,
this.classBuilder);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.classBuilder не нужен

}
}
},
'<div class="b1 b2__e b3 b3_test_opa b4 b5"></div>');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need test with templates on mixes

@miripiruni miripiruni closed this May 10, 2017
@miripiruni miripiruni deleted the issue-241__nested-mixes branch January 29, 2018 11:09
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

4 participants