Problem/Motivation

Menu classes are generated in menulinkthree.php instead of beeing variables that the theme then uses and creates the classes that it want
its cumbersome for a theme to change this by ex using .hasClass()

        set class = [
          'menu-item',
          item.attributes.hasClass('menu-item--expanded') ? 'expanded-to-max' : 'no-expandin-for-you',
          item.attributes.hasClass('menu-item--collapsed') ? 'collapsed' : 'not-collapsed',
          item.attributes.hasClass('menu-item--active-trail') ? 'ze-trail-mybroherts' : 'no-trail-dude',
        ]

Proposed resolution

remove the class array and exchange that with relevant variables.

      if ($data->hasChildren && !empty($data->subtree)) {
        $class[] = 'menu-item--expanded';
      }

to this

if ($data->hasChildren && !empty($data->subtree)) {
        $element['is_expanded'] = TRUE;
      }

template:
item.is_expanded ? 'menu-item--expanded'

Similar changes have been made to set variables is_collapsed and in_active_trail for both the menu and book-tree templates.

It gets visible for the themer where & what gets generated & we get the true separation of code & templates

Remaining tasks

- Review and commit

User interface changes

- None

API changes

- Adds three additional preprocess variables

Data model changes

- None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because this is primarily an improvement for where markup is generated in line with other changes to the theme system.
Issue priority Normal because this is a useful change for themers, but should not block release.
Disruption Not at all disruptive as the template output remains the same.
CommentFileSizeAuthor
#56 interdiff-menugen-51to56.txt835 bytesdavidhernandez
#56 menu_class_generating-2547159-56.patch15.85 KBdavidhernandez
#51 interdiff-2547159-45-51.txt5.95 KBRainbowArray
#51 2547159-51-menu-class-generating.patch15.84 KBRainbowArray
#45 interdiff-2547159-43-45.txt5.96 KBRainbowArray
#45 2547159-45-menu-class-generating.patch13.87 KBRainbowArray
#43 interdiff-2547159-40-43.txt3.72 KBRainbowArray
#43 2547159-43-menu-generating-classes.patch10.93 KBRainbowArray
#40 interdiff-2547159-33-40.txt2.73 KBRainbowArray
#40 2547159-40-menu-class-generating.patch7.84 KBRainbowArray
#33 interdiff-2547159-27-33.txt951 bytesRainbowArray
#33 2547159-33-menu-class-generating.patch7.52 KBRainbowArray
#30 interdiff-2547159-27-30.txt951 bytesRainbowArray
#30 2547159-30-menu-class-generating.patch7.52 KBRainbowArray
#28 interdiff-2547159-17-27.txt3.55 KBRainbowArray
#28 2547159-27-menu-class-generating.patch7.49 KBRainbowArray
#17 interdiff-2547159-13-17.txt3.07 KBRainbowArray
#17 2547159-17-menu-class-generating.patch4.92 KBRainbowArray
#13 menu_class_generating-2547159-13.patch4.91 KBmortendk
#8 interdiff.txt1.64 KBlauriii
#8 menu_class_generating-2547159-8.patch4.66 KBlauriii
#3 menu_class_generating-2547159-2.patch3.32 KBmortendk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mortendk created an issue. See original summary.

mirom’s picture

Assigned: Unassigned » mirom
mortendk’s picture

Status: Active » Needs review
FileSize
3.32 KB

ok testbot lets see how you like this

mirom’s picture

Assigned: mirom » Unassigned

Status: Needs review » Needs work

The last submitted patch, 3: menu_class_generating-2547159-2.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: menu_class_generating-2547159-2.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
1.64 KB
joelpittet’s picture

Status: Needs review » Needs work

Couple questions and notes:

  1. +++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
    @@ -32,7 +32,15 @@
    -        <li{{ item.attributes }}>
    +      {%
    +        set class = [
    +          'menu-item',
    +          item.is_expanded ? 'menu-item--expanded',
    +          item.is_collapsed ? 'menu-item--collapsed',
    +          item.is_active_trail ? 'menu-item--active-trail',
    +        ]
    +      %}
    

    Should this only go in classy?

  2. +++ b/core/themes/classy/templates/navigation/menu.html.twig
    @@ -29,8 +29,16 @@
    +    {% for item in items %}
    ...
    +        <li{{ item.attributes.addClass(class) }}>
    

    If this is to move back the whole <li> should be unindented as well.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
@@ -243,26 +243,24 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
+        $element['is_expanded'] = TRUE;
...
+        $element['is_collapsed'] = TRUE;
...
+        $element['is_active_trail'] = TRUE;

I'd have expected these to be #is_expanded etc., i.e. properties. But I guess that doesn't make sense here?

joelpittet’s picture

@Wim Leers they aren't render arrays they are just item hashes, the $element variable name is just there to throw you off your game;)

Wim Leers’s picture

the $element variable name is just there to throw you off your game;)

Indeed!

Just ignore #10, #11, #12 please — sorry for the distraction!

mortendk’s picture

Status: Needs work » Needs review
FileSize
4.91 KB

@joel yes it should only be in classy (and in the menu--toolbar) it is in classy where the classes should live.

RainbowArray’s picture

In #2553463: Move system link.theme.css, links.theme.css, menu.theme.css, and tabs.theme.css to Classy, we found there's still a menu class in menu.html.twig in core. It looks like the associated styles are purely aesthetic. Trying to figure out the best place to take care of moving that class to Classy. Unless there's a specific reason that's still in core?

mortendk’s picture

as I remember it the reason we still have .menu was some test issue ?
if that's not the case then everything goes to classy - but that's not this issue, this is about moving hardcoded classes to variables ;)

Wim Leers’s picture

+++ b/core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php
@@ -141,13 +141,25 @@ public function providerTestBuildCacheability() {
+      if ($ekement->inActiveTrail) {
+        $return['is_active_trail'] = TRUE;

Note "in" versus "is". Should be consistent.

RainbowArray’s picture

I figured out the menu class issue. Not relevant here. Yes, it was in the template, we pulled it out for a test issue, we fixed the test but never pulled that class out. That's being fixed in another issue.

This fixes a typo, the in vs is thing Wim pointed out and uses a classes vs class variable, which looks to be the pattern in other templates.

I just spent too many hours fighting problems just like this in D7. This looks like a great fix, it's straightforward, and I think we can move to RTBC on this one. Let's get this in!

RainbowArray’s picture

Also, it's right to keep those classes in menu--toolbar for core, as those need to be available to non-Classy-based themes as well, so the toolbar works everywhere.

davidhernandez’s picture

+++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
@@ -32,7 +32,15 @@
+          item.is_expanded ? 'menu-item--expanded',
+          item.is_collapsed ? 'menu-item--collapsed',

This is basically a boolean state, so why not just do something like:

item.state ? 'menu-item--' ~ item.state

Trying to think if that variable is useful for something more than just making a class.

RainbowArray’s picture

davidhernandez: I think state is a pretty generic thing where it's not at all clear what is in that. is_expanded and is_collapsed is very clear and will be easier to use by themers without having to look up the possible values of state, or the possibility that some other random thing gets thrown into the state variable because somebody is not clear on what it should be used for.

mortendk’s picture

the reason I used is_ is to keep it in the same feel as smacss have that use is for its state

RainbowArray’s picture

#21: +1

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
@@ -243,26 +243,24 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
-      $element = array();

Don't remove this, it resets the element in the array.

I think this looks good otherwise. I don't care either way on thie is_ vs in_. So sort it out between yourselves.

lauriii’s picture

Status: Needs work » Needs review

Its just moved few rows up.

RainbowArray’s picture

We didn't remove $element = array(), it just got moved up a little higher in the function.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

@mdrummond mentioned it moved up. (should have looked up).

Anyways this looks good as-is.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
+++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
@@ -32,7 +32,15 @@

@@ -32,7 +32,15 @@
       <ul class="toolbar-menu">
     {% endif %}
       {% for item in items %}
-        <li{{ item.attributes }}>
+      {%
+        set classes = [
+          'menu-item',
+          item.is_expanded ? 'menu-item--expanded',
+          item.is_collapsed ? 'menu-item--collapsed',
+          item.in_active_trail ? 'menu-item--active-trail',
+        ]
+      %}
+        <li{{ item.attributes.addClass(classes) }}>
           {{ link(item.title, item.url) }}
           {% if item.below %}

Edit: Moar context

Indent here needs to be moved in one like other one.

Also more importantly could we expand this issue to include \Drupal\book\BookManager.?
It has identical code or can someone make a follow-up?

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
7.49 KB
3.55 KB

This does the same thing for book-tree that we were doing for menu and fixes the indent too.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/book/src/BookManager.php
    @@ -539,30 +539,28 @@ protected function buildItems(array $tree) {
    -      // Generally we only deal with visible links, but just in case.
    

    Probably don't need to delete this comment.

  2. +++ b/core/modules/book/src/BookManager.php
    index 4165072..e8e6b45 100644
    --- a/core/modules/toolbar/templates/menu--toolbar.html.twig
    
    --- a/core/modules/toolbar/templates/menu--toolbar.html.twig
    +++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
    

    Nice find! That definitely needs to be in this issue.

+++ b/core/themes/classy/templates/navigation/menu.html.twig
@@ -29,13 +29,21 @@
+    {% for item in items %}
...
       {% endfor %}

I think you were right to move the for back but the end for needs to be inline with it still. It looks silly because the if/else above it indents the ul.

RainbowArray’s picture

RainbowArray’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: 2547159-30-menu-class-generating.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
951 bytes

Redid the patch. Something odd may have happened.

Status: Needs review » Needs work

The last submitted patch, 33: 2547159-33-menu-class-generating.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

So question, since the other ones don't touch core/stark should this template be duplicated in classy instead?

I'm ok with leaving it like this because technically that's what's in core right now. But thought I'd ask.

+++ b/core/modules/toolbar/templates/menu--toolbar.html.twig
@@ -32,7 +32,15 @@
-        <li{{ item.attributes }}>
+        {%
+          set classes = [
+            'menu-item',
+            item.is_expanded ? 'menu-item--expanded',
+            item.is_collapsed ? 'menu-item--collapsed',
+            item.in_active_trail ? 'menu-item--active-trail',
+          ]
+        %}
+        <li{{ item.attributes.addClass(classes) }}>

classy?

RainbowArray’s picture

No because the toolbar needs to work in non-classy themes. So all themes need those classes.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation, +needs issue

Sorry to kick this back but the fixes are easy.

  1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -243,26 +243,24 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
    +      $element = array();
    
    +++ b/core/modules/book/src/BookManager.php
    @@ -539,30 +539,29 @@ protected function buildItems(array $tree) {
    +      $element = [];
    

    Minor: I guess we can use short array syntax on the first one too.

  2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -243,26 +243,24 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
    +        $element['is_expanded'] = TRUE;
    ...
    +        $element['is_collapsed'] = TRUE;
    ...
    +        $element['in_active_trail'] = TRUE;
    

    I thought elsewhere we were initializing these as FALSE for easier debugging.

    Example from template_preprocess_views_view_table():

      $variables['sticky'] = FALSE;
      if (!empty($options['sticky'])) {
        $variables['view']->element['#attached']['library'][] = 'core/drupal.tableheader';
        $variables['sticky'] = TRUE;
      }
    
  3. +++ b/core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php
    @@ -141,13 +141,25 @@ public function providerTestBuildCacheability() {
         $get_built_element = function(MenuLinkTreeElement $element, array $classes) {
    -      return [
    -        'attributes' => new Attribute(['class' => array_merge(['menu-item'], $classes)]),
    +      $return = [
    +        'attributes' => new Attribute(),
    

    If we remove all uses of $classes from this function thingy then we should probably remove it from the $get_built_element line.

star-szr’s picture

Issue tags: -needs issue +Needs issue summary update
RainbowArray’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
2.73 KB

This should fix the items in #38.

Status: Needs review » Needs work

The last submitted patch, 40: 2547159-40-menu-class-generating.patch, failed testing.

The last submitted patch, 40: 2547159-40-menu-class-generating.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
10.93 KB
3.72 KB

Tested this locally, seems to works now. We removed the classes array from the function definition but not from where the function was called. That's fixed now. Also set up the defaults in the unit test as well.

star-szr’s picture

Status: Needs review » Needs work

Thanks @mdrummond! I think below is all I can find now and then this is ready from my perspective after it gets an issue summary update (and beta evaluation).

  1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -243,26 +243,27 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
    +      // Set a helper variable to indicate whether link is in active trail.
    
    +++ b/core/modules/book/src/BookManager.php
    @@ -539,30 +539,32 @@ protected function buildItems(array $tree) {
    +      // Set a helper variable to indicate whether link is in active trail.
    

    This is a bit terse, can we say something more like: "…whether the link is in the active trail."

    (would need to be wrapped to 80 chars).

  2. +++ b/core/modules/book/src/BookManager.php
    @@ -539,30 +539,32 @@ protected function buildItems(array $tree) {
    +      // tasks, only set 'expanded' true if the link also has children within
    

    Again a bit terse, "…only set 'expanded' to true…" would be an improvement I think.

  3. +++ b/core/themes/classy/templates/navigation/menu.html.twig
    @@ -29,14 +29,22 @@
    +          item.is_expanded ? 'menu-item--expanded',
    +          item.is_collapsed ? 'menu-item--collapsed',
    +          item.in_active_trail ? 'menu-item--active-trail',
    

    Let's document these three new sub-variables in all relevant menu and book-tree templates please.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
13.87 KB
5.96 KB

This should fix the doc changes requested in #44.

RainbowArray’s picture

Issue summary: View changes
RainbowArray’s picture

Issue summary: View changes
dawehner’s picture

I really like the patch as it is. Its one small step from moving towards view objects.

davidhernandez’s picture

Doing some testing.

davidhernandez’s picture

Status: Needs review » Needs work

I'm mostly testing in Classy. Stark has no classes for these templates so the only difference I see is whether the right comments have been added to the templates.

Enable book and generate book content. Added nested book items.
Enable Classy and add Administration menu block and Book navigation block.
Modify Admin menu to have several items expanded and collapsed.
Checking markup and classes added. Copied the markup for both and toolbar menu for reference.
Verify that all three menu types have expanded, collapsed, and active trail.

Apply patch.
Checking book menu, no difference.
Checking admin menu, no difference.
Checking toolbar, no difference.

I diffed the output from all three and did not find a difference. Since the class generation is the same, CSS shouldn't be affected.

A couple things I noticed, none are big.

1) I noticed the toolbar never sets active trail. Is this a bug or by design? It is that way in head, so not something fix here. Just something I noticed.

2) The indentation of the templates is inconsistant in one regard. The for loop after the endif is indented in all the different templates, but unindented in Classy's menu template. Unindented is actually correct. Regardless of wich way, we should make them all the same.

3)

+++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
@@ -243,26 +243,28 @@ protected function buildItems(array $tree, CacheableMetadata &$tree_access_cache
+      // Set a variable for the <li>-tag. Only set 'expanded' to true if the

Why is this hyphen here after the li tag? It's in the original, but I don't think I've seen it written that way anywhere else.

Otherwise, I'm inclined to RTBC it.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
15.84 KB
5.95 KB

This fixes the issues mentioned in #2 and #3. As #1 indicates, if the active trail wasn't being set in the toolbar prior to this patch, I don't think we'd want to address that here. I'm not sure you would want an active indicator in the toolbar, but either way, that does not seem in scope here.

Consistency is in scope, though, so thanks for the finds, davidhernandez! Much appreciated.

Status: Needs review » Needs work

The last submitted patch, 51: 2547159-51-menu-class-generating.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
davidhernandez’s picture

Hmm. One nitpick. The comments are not the same between the two menu templates. System and Classy. It looks like one was maybe copied from the book template and the work book was removed, leaving the sentences different.

+++ b/core/modules/system/templates/menu.html.twig
@@ -11,6 +11,11 @@
+ *   - is_collapsed: TRUE if the link has children within the current menu tree
+ *     that are not currently visible.

In the system template it specifies menu tree.

+++ b/core/themes/classy/templates/navigation/menu.html.twig
@@ -11,6 +11,11 @@
+ *   - is_collapsed: TRUE if the link has children within the current tree that
+ *     are not currently visible.

Here it does not. The toolbar template also says "menu tree".

davidhernandez’s picture

Here is the text change. If it's ok I'll rtbc.

RainbowArray’s picture

Status: Needs review » Reviewed & tested by the community

Green, so rtbc based on above comment.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: menu_class_generating-2547159-56.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review

Seems like a random unrelated test fail.

RainbowArray’s picture

Status: Needs review » Reviewed & tested by the community

Drupal CI is green. Moving this back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.