common.blocks: prepare for bem-xjst@2.x #1495
Conversation
| delete this._menuItemDisabled; | ||
| this.mods.disabled = true; | ||
| apply(); | ||
| return applyNext(); |
There was a problem hiding this comment.
здесь точно надо apply менять на applyNext?
There was a problem hiding this comment.
@Guria для новой версии bem-xjst нужны такие изменения — по семантике это вполне корректно, просто раньше мы "экономили" applyNext т.к. был кастомный гард про _menuItemDisabled
There was a problem hiding this comment.
@zxqfox да, т.к. он нужен по смыслу
veged
force-pushed
the
feature/prepare-for-bem-xjst-v2
branch
3 times, most recently
from
642411f to
6677f82
Compare
| var checkedOptions = this._checkedOptions, | ||
| firstOption = this._firstOption; | ||
| if(firstOption && !checkedOptions.length) { | ||
| firstOption.checked = true; | ||
| checkedOptions = [firstOption]; | ||
| } | ||
| applyNext({ _checkedOption : checkedOptions[0] }); | ||
| return applyNext({ _checkedOption : checkedOptions[0] }); | ||
| }), | ||
|
|
||
| content()(function() { |
There was a problem hiding this comment.
when compiled with bem-xjst@2.x, this._checkedOption is undefined in content()() for some reason
// cc @indutny
There was a problem hiding this comment.
How did you test it? Could it be that _checkedOptions was undefined too?
There was a problem hiding this comment.
looks like it is defined at https://github.com/bem/bem-components/blob/feature/prepare-for-bem-xjst-v2/common.blocks/select/select.bemhtml#L34 and undefined in block('select').mod('mode', 'radio')
There was a problem hiding this comment.
So it was present in select.bemhtml? It is especially interesting to see, because this block appears to be present in the one of the desktop.pages, and I have verified that generated html is the same with old and new bem-xjst.
There was a problem hiding this comment.
please take a look at https://github.com/tadatuta/select-template-issue-demo
not the very best test case but hope it will help
There was a problem hiding this comment.
@tadatuta very interesting!
@veged the situation is following: there are block('select').mod('mode', 'radio') which has higher priority than block('select').def() in the .bemhtml file. When block('select').def() calls applyNext() - it is only invoking the matches in the blocks with lower priority than block('select').def(), skipping the block('select').mod('mode', 'radio').
Looks like our idea about incrementing index and invoking next templates is not as viable as we thought it will be. What do you think?
There was a problem hiding this comment.
@tadatuta may I ask you to give a try to the latest bem-xjst 2.x master? It might be fixed by bem/bem-xjst@ee7d1de
There was a problem hiding this comment.
yes, this issue is resolved, thanks!
|
Is it WIP? |
|
@indutny ping! |
|
@narqo I think all issues are resolved now. Feel free to land it if it LGTY |
|
I think we should merge bem/bem-core#1022, release a version and then update dependency. Otherwise there's just no reason. |
|
It doesn't seem like it is blocking this one, or am I missing something? |
|
it's not blocking but it's kinda useless without support in |
|
@tadatuta do we want to merge this, considering that bem/bem-core#1022 was merged? |
|
yeah. but looks like rebase is needed. |
|
@indutny rebase and merge, please. |
aristov
force-pushed
the
feature/prepare-for-bem-xjst-v2
branch
from
9f81d64 to
7a905fb
Compare
common.blocks: prepare for bem-xjst@2.x
match()with inline valueapplyNext()/applyCtx()cc @veged