Make menu.html.twig easier to understand

Thanks for this issue, but it does not look like a cool. IMHO, here it looks cool:

{% if items %}
    # set attributes by conditions
    <ul{{ attributes }}>
    {% for item in items %}
        <li>…
    {% endfor %}
    </ul>
{% endif %}

And the page of TWIG documentation does not describe 'for + loop variable' as a pattern for this case. But I found another page, where in the second sample uses.. if for this case :)

I hope my comment will understand correctly.

Yeah, you're right! Although I have a few ways to create it, but their implementation is not real in this world)

1. The Ugly: just clone, and unset all property

{% set new_attributes = attributes %}
{% set new_attributes = new_attributes.__clone() %}
{% for key in new_attributes.storage()|keys %}
    {% set _ = new_attributes.offsetUnset(key) %}
{% endfor %}

...
{% set new_attributes = new_attributes.addClass('menu') %}

2. The Bad: add new method to the core/lib/Drupal/Core/Template/Attribute.php

public function instance($attributes = array()) {
    return new Attribute($attributes);
}

#twig
{% set new_attributes = attributes.instance() %}
{% set new_attributes = attributes.addClass('menu') %}
# or
{% set new_attributes = attributes.instance({'class': 'menu'}) %}

3. The Good: architectural solution in Drupal, giving their own attributes for all sub-menu (not only for root menu).

#using idea:

{% import _self as menus %}
{{ menus.menu_links(items, menu_attributes, 0) }}

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    <ul{{ attributes }}>
    {% for item in items %}
      <li{{ item.attributes }}>..
        {% if item.below %}
          {{ menus.menu_links(item.below, item.menu_attributes, menu_level + 1) }}
        {% endif %}
      </li>
    {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

But the goal of my post #9 was just to draw attention to the fact that

for 
  if
    if
  code
  if

is worse than

if
  if
  for
    code

Thanks!

new Twig function - a great idea! It seems that it will be useful not only in these menu.twig templates. Unfortunately the pure name 'attribute' is occupied of Twig, but we can use drupal way pefix 'drupal_attribute'.

Examples:

{% set attributes = drupal_attribute() %}
{% set attributes = drupal_attribute().addClass('menu') %}
{% set attributes = drupal_attribute({'class': 'menu'}) %}

And nit refactoring

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
      <ul{{ attributes }}>               {#!!!#}
        {% for item in items %}
            <li{{ item.attributes }}>
                {{ link(item.title, item.url) }}
                {% if item.below %}
                    {% set attributes = drupal_attribute().addClass(['menu']) %}        {#!!!#}
                    {{ menus.menu_links(item.below, attributes, menu_level + 1) }}
                {% endif %}
            </li>
        {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

also we can use two macro for rendering: wrapper (ul), content (li). Maybe it's better for cleaning twig template (at least, it's better than the loop ;)

Regarding 'The Good'. Yeah, it will make some things done qualitatively. But it also adds flexibly manage by sub-menu (programmatically and via the UI). I sometimes need this opportunity for creative menu. But now it ossified piece for dull menu :)

We can not moving the /ul outside of the "for" loop, because if empty items the result:
</ul>

The idea of #1 - use only check by {% for item in items %} without {% if items %}. Hence all code must be inside loop. But we are very costly to pay for it. And not only because of the extra performance conditions within the loop. The pattern itself is not successful.

It's not look like:

wrapper_start
    template
wrapeer_end

It's look like:

children_wrapper_start
  parent_wrapper_start
  template
  parent_wrapper_end
  template
  parent_wrapper_start
  template
  parent_wrapper_end
children_wrapper_end

It's antipattern. We inject parent code into children. If we want clear template, maybe better use two macro, like this:

{% macro menu(items, attributes, level) %}
{% if items %}
    {# start template #}
    <ul{{ attributes }}>
        {{ menus.menu_items(items, attributes, level) }}
    </ul>
    {# end template #}
{% endif %}
{% endmacro %}


{% macro menu_items(menu_items, menu_attributes, menu_level) %}
    {% for item in menu_items %}
    {# start template #}
        <li{{ item.attributes }}>..
    {# end template #}
    {% endfor %}
{% endmacro %}

But I am satisfied with the fact that we have now. We just need to solve the problem of double-ul and assigning attributes.

#17: I think this issue primarily focus about improving the template of menu. Loop - only one variant (and not the best). My english is very poor, and I'm tired of pointing this out. Hopefully, John Albin and all others correctly sees my attempt to help. Thanks and good luck!

What's up?)

After #2616756: Allow instantiating Attribute objects within Twig, we can solve problem with double-ul like this:

<ul{{ attributes.addClass(['menu']) }}>
....
{{ menus.menu_links(item.below, create_attribute(), menu_level + 1) }}

But if we need some fresh ideas, how to make the menu.html.twig easier to read.. Gentlemen, what do you think, if we apply the pattern Mediator (or maybe it's called a Facade, Bridge, .. your variant here). This can well help separate the logic from the layout:

{% import 'core/modules/system/templates/menu_render.twig' as render %}
{{ render.menu(_self, items) }}

{% macro menu(render, items, attributes) %}
    <ul{{ attributes }}>
        {{ render.links(_self, items) }}
    </ul>
{% endmacro %}

{% macro link(render, item, attributes) %}
    <li{{ attributes }}>
        {{ link(item.title, item.url) }}
        {{ render.menu(_self, item.below) }}
    </li>
{% endmacro %}

Expanded variant:

 {% import 'core/modules/system/templates/menu_render.twig' as render %}
 {{ render.menu(_self, items, attributes, 0) }}
 
 {% macro menu(render, items, attributes, menu_level) %}
+    {% set classes = ['menu'] %}
+    {% set attributes = attributes.addClass(classes) %}
     <ul{{ attributes }}>
         {{ render.links(_self, items, menu_level) }}
     </ul>
 {% endmacro %}

 {% macro link(render, item, attributes, menu_level) %}
+  {%
+  set classes = [
+  'menu-item',
+  item.is_expanded ? 'menu-item--expanded',
+  item.is_collapsed ? 'menu-item--collapsed',
+  item.in_active_trail ? 'menu-item--active-trail',
+  ]
+  %}
+  {% set attributes = attributes.addClass(classes) %}
   <li{{ attributes }}>
     {{ link(item.title, item.url) }}
     {{ render.menu(_self, item.below, create_attribute(), menu_level + 1) }}
   </li>
 {% endmacro %}

And magic core/modules/system/templates/menu_render.twig:

{% macro menu(template, items, attributes, menu_level) %}
  {% if items %}
    {% if not attributes %}
      {% set attributes = create_attribute() %}
    {% endif %}
    {{ template.menu(_self, items, attributes, menu_level) }}
  {% endif %}
{% endmacro %}

{% macro links(template, items, menu_level) %}
  {% for item in items %}
    {{ template.link(_self, item, item.attributes, menu_level) }}
  {% endfor %}
{% endmacro %}

Edit: correct mistake

- {% set item.attributes = item.attributes.addClass(classes) %}
+ {% set attributes = attributes.addClass(classes) %}

Hence now macro link have new param attributes. Because it item.attributes = ... not work, sorry!

Also, it may need to supplement the documentation of the available variables:

  * Available variables:
+ * - menu_name: machine name of the menu
+ * - attributes: HTML attributes for the menu,
+ *   instance of \Drupal\Core\Template\Attribute
  * - items: A nested list of menu items. Each menu item contains:
  *   - ...
+ *   - original_link: instance of \Drupal\Core\Menu\MenuLinkDefault