API page: https://api.drupal.org/api/drupal/core%21modules%21system%21templates%21...

Problem/Motivation

Right now it is not obvious that menu_name is only globally available and it needs to get passed into the menu_links template menu.html.twig to render e.g. in the toplevel ul.

This is probably only necessary for themers / developers who hit the first time a twig macro (was it for me) but menu.html.twig might be one of the first templates with which a themer / developer gets in contact and would ease the usability of the template.

I described my issue here and @joelpittet cleared it up.

Proposed resolution

Mention in the template header / comments that menu_name needs to get past into the menu_links macro because it is only globally available like items.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

4aficiona2 created an issue. See original summary.

4aficiona2’s picture

Title: Mention only global available in menu.html.twig comments » Mention in the comments that menu_name is only global available in menu.html.twig
4aficiona2’s picture

Category: Bug report » Task
joelpittet’s picture

Thanks for creating this issue. One thing to note is likely you wouldn't be using that menu_name class on every ul/li. So more practically speaking you should use it outside the macro surrounding the call to the macro.

eg.

<menu class="{{ menu_name }}">{{ menus.menu_links(items, attributes, 0) }}</menu>

Or better:

<menu{{ attributes.addClass(menu_name|clean_class) }}>{{ menus.menu_links(items, attributes, 0) }}</menu>
4aficiona2’s picture

This would be one option, but if you want to use it within the toplevel ul which has already a special case in the template (also no need for an extra wrapper element) it would still be necessary to pass it into the macro like below.

{% if menu_level == 0 %}
  <ul{{ attributes }} {{ attributes.addClass(menu_name) }}>

Either way, it might be good to mention that menu_name is only globally available for twig newbies (like me).

joelpittet’s picture

Issue tags: +Novice, +Twig

Do you have a proposal for how the comment should read? I'd like to have something short and to the point that can explain that since it's really the only macro used in core (except for the variations for book and toolbar which do the same thing).

4aficiona2’s picture

Sure, I completely understand. My suggestion would be:

Old comment:
* - menu_name: The machine name of the menu.

New comment:
* - menu_name: The machine name of the menu (if needed within macro like items, pass it too into the macro as an argument).

In my opinion this gives an extra hint to be aware that menu_name needs to get passed in the macro even if it does not mention explicitly that menu_name and items are only globally available. This could be an option too to mention that those two are only globally available but I tried to keep it as short as possible. I know, its over 80 chars.

I hope this is a viable solution. What is your opinion @joelpittet? Too much of an edgecase?

Cottser’s picture

Yeah this is a tricky one, we don't want to confuse things either. I would lean towards more of a message saying that menu_name isn't passed into the macro by default. There are three places that you'd need to change to make menu_name available in all levels of the macro so not sure how we explain that.

Or what if we just pass menu_name into the macro so it is available? Would that break anything? Macro arguments are optional so seems like it would be fine…

4aficiona2’s picture

Sure, if menu_name also gets passed into the macro then the additional comment is obsolet. items and menu_name would be available outside and within the macro which was what I initially assumed (might others too which face a twig macro for the first time too).

And in my opinion it should not break the existing templates. If it gets outputted too, e.g. on the toplevel ul, is another question and should be up to the themer. But in my case I like to output menu_name as a class name to distinguish the different menus without an extra wrapper element.

Cottser’s picture

@4aficiona2 if you're up for it I think uploading a patch changing all the menu and menu-like templates to include menu_name as relevant would be a good next step. We can at least see if any of the core automated tests fail as a result of that, and go from there.

laughnan’s picture

Don't know if we are leaning toward just updating the comment (see attached). As a pseudo novice to the theme layer (and not finding documentation specifically on it), where are the twig macros to modify?

laughnan’s picture

And whoops. My comment patch was supposed to be geared more toward the 'not by default' response. Updating.

joelpittet’s picture

@laughnan re #11 the macro is the function looking thing in that file.
http://twig.sensiolabs.org/doc/tags/macro.html

We've opted to keep it in the menu.html.twig and self import it as to avoid finding some generic place to put it and still have it available to the theme system template overrides.

The documentation patch will be easy to change in > 8.0.x. If we actually change the macro that will be 8.1.x material and may not be worth your while as the use-case for it may be slim(I could be wrong here). I'm completely on board with clear documentation to help people out though.

Cottser’s picture

Adding it to the macro might be more of a curiosity at this point, but also interesting to think about best practices for macros and how much variables should we be passing in. Or should we always pass in the _context like the docs show, that's another option.

My new idea for the docs side is to use @code to show how the macro would need to be changed. As I said three lines need to be changed and I don't think trying to explain that in words will be effective.

laughnan’s picture

Thanks for that @joelpittet. Forever learning, forever learning...

joelpittet’s picture

Aren't we all;)

radubutco’s picture

Assigned: Unassigned » radubutco
Issue tags: +SprintWeekend2016, +SprintWeekendTM
radubutco’s picture

Assigned: radubutco » Unassigned
Status: Active » Needs review
FileSize
1015 bytes

I've added a patch which passes the menu_name into the macro, as mentioned in #8.
Please review.

iamartin’s picture

Test should be performed using Stark theme because fix has already been applied to Classy, which rolls down to Bartik.

Test passed.

joelpittet’s picture

Status: Needs review » Needs work

@radubutco I don't think we should change this. My thought is that it's not common need to add the menu name to all ul/li/a and that could cause some CSS cascading issues in styles due to the recursiveness of this macro. So I'd much rather like the title says, add notes to why it's not available.

tstoeckler’s picture

Issue tags: +DrupalBCDays

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

thamas’s picture

To inform those who are arrive here because of facing the problem of using menu_name I wrote a blog post related to the problem (and using the information) of this issue: https://medium.com/integral-vision/drupal-8-twig-add-custom-css-classes-...

example_mentee’s picture

Based on joelpittet's comment in #20, The scope of this issue should be to only add a comment in the twig template that explains that {menu_name} can be added to the Twig call. The patch should not actually attempt to change the default Twig code.

Novice tasks:

  • The Issue summary should be updated, and
  • a new patch should be created.
laughnan’s picture

@example_mentee is then comment/patch #12 sufficient?

JohnAlbin’s picture

I don't think the patch in #12 is sufficient to explain the actual problem.

This patch adds the comment where the problem exists, inside the macro itself.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
2 KB

Hmm… http://twig.sensiolabs.org/doc/tags/macro.html uses the term "arguments" instead of "parameters". We should keep the terminology the same. Here's a corrected patch.

thamas’s picture

@JohnAlbin Maybe it would be nice to help newbies by mentioning that we are thinking of the variables listed in the intro comment of the file. Or is it obvious?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dimaro’s picture

Add test against 8.2.x on #27

vanessakovalsky’s picture

Status: Needs review » Reviewed & tested by the community

This patch seems good to me and apply without problems

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/templates/menu.html.twig
@@ -29,6 +29,11 @@
+    access aditional variables, add more arguments to this macro; or see

s/aditional/additional/

Perhaps we should add to this documentation that already exists in these files?

{#
  We call a macro which calls itself to render the full tree.
  @see http://twig.sensiolabs.org/doc/tags/macro.html
#}

I'm actually not sure what our policies are on this, but I wonder if we should consider creating docs on drupal.org and linking to them from these files? It would be easier to provide sample code and explain recursion, scope, etc. in that format, rather than a small inline comment.

dimaro’s picture

Patch to change aditional for additional.

Perhaps we should add to this documentation that already exists in these files?

{#
  We call a macro which calls itself to render the full tree.
  @see http://twig.sensiolabs.org/doc/tags/macro.html
#}

@Cottser as you said on #32 the documentation already exists in these files.
We should change the latest patch to remove the extra documentation and finally do something like this?:

+++ b/core/modules/system/templates/menu.html.twig
@@ -29,6 +29,11 @@
+  {#
+    This macro can only access the variables passed to it in its arguments. To
+    access aditional variables, add more arguments to this macro.
+  #}
Cottser’s picture

@dimaro thanks! I'm suggesting we consider expanding that existing documentation. So not removing documentation but adding/combining with the existing docs in these files.

dimaro’s picture

@Cottser thanks for the explanation.
It may be interesting to include all the information.
However, I think that we should update the scope of this issue to do that, right?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.