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

[menu] base template for block contains logic only for one mod. #1513

Closed
Guria opened this issue May 26, 2015 · 9 comments
Closed

[menu] base template for block contains logic only for one mod. #1513

Guria opened this issue May 26, 2015 · 9 comments

Comments

@Guria
Copy link
Contributor

Guria commented May 26, 2015

da75b81 introduced logic which demanded only by menu_mode_radio template.
It iterates over content to set properties _firstItem and _checkedItems. Iteration function has assumption that menu should contain only menu-item or menu__group. It very inconvinient when we try to put inside block menu another elements or blocks which will be replaced with menu-item in later processing.

I suppose two enhancements:

  • Move that specific code under mod('mode', 'radio') matcher.
  • Make content iteration function indiferrent to block's content:
    • add ctx.content = [].concat(ctx.content) instead of if(!this.isArray(ctx.content)) throw Error
    • check on content property existance (else if(ctx.content) { // menu__group) before recursive call

Additionally we can support case of resolving blocks to menu-item which cannot be catched by iteration function and add js behavior in init section of menu_mode_radio, and document that in such cases live initialization should be disabled. But I think it can and should be done at project level.

/cc @hcodes @tadatuta

@Guria
Copy link
Contributor Author

Guria commented May 26, 2015

I can provide a PR if you accept my proposal.
Also I suppose to block #1503 because such changes also will require a minor version bump.

@aristov
Copy link
Contributor

aristov commented May 26, 2015

// cc @narqo

@narqo
Copy link
Member

narqo commented May 26, 2015

The main purpose of changes from da75b81 is this exact line. E.g. we need to map values from val property to nested menu-items. We can't make it for _mode_radio only.

The same thing goes to if(!this.isArray(ctx.content)) throw Error question. ctx.content as an Array is the part of API, that we can guarantee. What kind of guarantees can be given in cases where literally everything can be passed as a input? Currently content is explicitly described in docs as "The array of the menu items".

I think for your particular case, the only "right" solution is to redefine def mode from menu block, and make processing of the input on the level of your project.

@Guria
Copy link
Contributor Author

Guria commented May 26, 2015

I have tried and didn't find an acceptable solution. I can't just skip def mode from bem-components, because I need somehow to get def from bem-core's i-bem.bemhtml. Could you provide an example?

@Guria
Copy link
Contributor Author

Guria commented May 26, 2015

I can accept your point but still ask one little change which will resolve my issue: add check of existence of ctx.content before iterateItems(itemOrGroup.content) call.
Please let us make great things on top of bem-components. This little change will help to adopt menu block in more cases.

@Guria
Copy link
Contributor Author

Guria commented May 27, 2015

The only way I managed to solve my issue:

{
  block: 'test',
  content: {
    elem:'menu',
    content: {
      block: 'test', // must explicitly set to preserve original block
      elem: 'items'
    }
  }
}
block('test')(
    elem('menu')(
        def()(function(){
            applyNext({
                _items:this.ctx.content, // hide content from bem-components menu template 
                _mode: '', // reset _mode like it does applyCtx
                'ctx': {
                    block: 'menu'
                }
            })
        })
    ),
    elem('items').def()(function(){
        return applyCtx([{
            block: 'menu-item',
            content: '1'
        },{
            block: 'menu-item',
            content: '2'
        }])
    })
);
block('menu')
    .match(function(){
        return !!this._items
    })
    .content()(function(){
        return this._items // restore content from _items
    });

to finally get

<div class="test">
  <div class="menu menu__control i-bem" data-bem="{&quot;menu&quot;:{}}" role="menu" tabindex="0">
    <div class="menu-item i-bem" data-bem="{&quot;menu-item&quot;:{}}" role="menuitem">1</div>
    <div class="menu-item i-bem" data-bem="{&quot;menu-item&quot;:{}}" role="menuitem">2</div>
  </div>
</div>

@Guria
Copy link
Contributor Author

Guria commented May 27, 2015

See also #1516.

@Guria
Copy link
Contributor Author

Guria commented May 27, 2015

This whole iterateItems thing really hard to understand without deep debugging.
I wasn't able to understand if expression from menu.bemhtml and very similar expression from menu_mode_radio.bemhtml does the same or a different things.

@narqo
Copy link
Member

narqo commented May 27, 2015

The only way I managed to solve my issue [..]

I'm still not sure I understand, what exactly you're doing. Could you show some real example?

I wasn't able to understand if expression from menu.bemhtml and very similar expression from menu_mode_radio.bemhtml does the same or a different things.

Actually they do the same things, but from the point of how menu_mode_radio should behave. There is an additional predicate in the template for _mode_radio. If no items have been checked, _mode_radio must check it's first item by itself, as it can't exists with out one, by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants