Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Please implement a feature to apply at least class
attribute to the entire menu (=the <ul>
element). This would allow to share styling among menus (such as when you have separate menus for each language).
Comment | File | Size | Author |
---|---|---|---|
#50 | menu_attributes-theme_override.jpg | 34.92 KB | MediaFormat |
#50 | menu_attributes-children.jpg | 34.25 KB | MediaFormat |
#50 | menu_attributes-ul_attributes.jpg | 38.19 KB | MediaFormat |
#43 | interdiff-1862048-30-42.txt | 919 bytes | skorzh |
#43 | menu_attributes-ul_attributes-1862048-42.patch | 3.72 KB | skorzh |
Comments
Comment #1
Exploratus CreditAttribution: Exploratus commentedI need that also.
Comment #2
rocbrook CreditAttribution: rocbrook commented+1!
I created a custom subtheme of Bootstrap and would love to be able to create stacked menus. To do so, I need to add the "nav nav-stacked" class to the ul tag. Currently, and amazingly frustratingly, this incredibly simple task cannot be done easily or without hacking code in Drupal.
Comment #3
meecect CreditAttribution: meecect commented+1!
This would make my life a lot easier. I would be happy just with the 'class' option, similar to how the block_class module works.
Comment #4
gusans CreditAttribution: gusans commented+1
This would be great for using with bootstrap nav classes.
Comment #5
jdcollier CreditAttribution: jdcollier commentedThis would be wonderful!
Comment #6
Nilard CreditAttribution: Nilard commented+1
Comment #7
batigol CreditAttribution: batigol commentedNot only a class but ID to. Just like menu items (li).
We really need menu (ul) to have and ID and class cause of :target element!
Comment #8
bdanin CreditAttribution: bdanin commentedThis would be helpful for me as well (adding classes to
<ul>
and<li>
menu elements). I'm surprised it's not already in this module.Comment #9
bdanin CreditAttribution: bdanin commentedis this a duplicate of https://drupal.org/node/1488960 ?
Comment #10
jwilson3@bdanin, as I read it, this is not a duplicate of #1488960: Attributes for LI element, which speaks of allowing *menu-item* classes to move from the
<a>
tag to the<li>
wrapper tag.This issue on the other hand seems to be primarily about exposing a setting (eg on the menu settings page) to specify a class and/or ID for an entire menu's
<ul>
tag.From a traditional themer point of view this seems like a ridiculous request because normally one would be able to style any menu using the existing classes provided on the wrapper divs in a theme, but with the advent of frameworks like bootstrap, i can see that it does make sense to expose the class/id attributes for the
<ul>
itself in some cases.Comment #11
bdanin CreditAttribution: bdanin commentedIt seems that with bootstrap using either LESS or SASS, it would be easy to extend an already existing class to the specific
<ul>
item without needing to append a class there. Although adding a class there might be faster / easier, especially for those that don't have access to compile the CSS. At this point, it would probably make more sense to add that ability to the D8 version.Comment #12
batigol CreditAttribution: batigol commentedIt would be very helpful in D7 as well. Default classes like menuitem1, menuitem2, menuitem3 + active, first, last etc. is a must in any cms.
Comment #13
bdanin CreditAttribution: bdanin commentedHere's one way I've done this in the past, it uses a couple very well maintained modules and I haven't experienced conflicts:
1) Use menu_block
2) Use block_class
Add the class to the menu block where needed. It doesn't add it to the
<ul>
item directly, but you can apply classes to entire menus this way very quickly and easily.Comment #14
jwilson3@#13, those are excellent workarounds, but without the ability to actually modify the class on the
<ul>
, they remain just workarounds. This is still a valid feature request that belongs in this module.Comment #15
joelpittetComment #16
skorzhI've created a patch what adds fields 'Id' and 'Classes' to menu edit form. These attributes will be added to the menu.
Patch relevant for 7.x-1.x-dev version.
Comment #17
batigol CreditAttribution: batigol commented@korgin, thanks! I'm gona test it next weekhend.
Comment #18
skorzhStill no updates here ;(
Comment #19
batigol CreditAttribution: batigol commented@korgik, sorry forget to give a review - everything works very well!
Comment #20
SpleshkaThanks @korgik! Patch itself looks good. I also have tested it at simplytest.me, and it works as expected. Nice job!
Comment #21
AndrewsizZ CreditAttribution: AndrewsizZ commentedHey,
Exactly what I need and works!
Comment #22
joelpittetThis looks good, I may put that in after the version 1.0 stable release which should be fairly shortly if the other maintainers don't object.
There are a few coding standard fixes that are needed in this patch. Maybe one of you could look at fixing those in and updated patch?
Here's a quick check based on https://drupal.org/coding-standards
single quotes.
Please don't use the short version of the name.
$attrs => $attributes
.Shouldn't these be translated as well?
attrs vs attributes again.
Ending comma on on last array item.
"Prepares variables for theme_menu_tree()."
@see https://www.drupal.org/node/1913208
Use $variables instead of $vars.
Implements instead of Override. and doesn't need the description under because all theme functions output markup.
Comment #23
skorzhThank you for review, I will fix
Comment #24
skorzhNew patch and interdiff attached.
Pay please attention, I've added
array_filter()
tomenu_attributes_menu_tree()
in order to remove empty array items.Comment #25
MediaFormat CreditAttribution: MediaFormat commentedGreat work!
I've just manually tested the patch,
my only feedback is that child or submenus are also given the menu attributes.
Duplicate IDs are problematic, and I'm not sure classes need the redundancy.
Probably just an oversight!
Thanks
Comment #26
skorzhThanks for feedback, yeah, seems you are right, I will check it
Comment #27
joelpittetThis will be much easier in D8 with
@see View souce https://api.drupal.org/api/drupal/core!modules!system!templates!menu.htm...
The UL and the LI are in the same template and have access to the depth in a recursive twig macro.
Unfortunately
theme_menu_tree()
doesn't have depth context which is super annoying for that issue in #25Comment #28
skorzhD8 is good;) but would be nice to have this feature in D7 too. So, my another attempt ;)
Now attributes will be applied only for top level of the menu.
Anybody test it please.
Comment #29
joelpittetNice trick! Depth context provided!
Class's values needs to be an array (true for d8 and d7)
Should be:
$attributes = array('class' => array('menu'));
To keep with existing styles should you merge the 'menu' class on there as well?
Comment #30
skorzh3. No, I've added 'menu' as default value for 'Classes' field, so if current menu wasn't store in 'menu_attributes_menus' var, then it will have default 'menu' class, otherwise it will use classes submitted from 'Classes' field (it had 'menu' as default value).
Comment #31
MediaFormat CreditAttribution: MediaFormat commentedTested patch #1862048-30: Class attribute for entire menu,
Works well.
Thank you @korgik
Comment #32
skorzhNo updates here?
Comment #33
MediaFormat CreditAttribution: MediaFormat commentedAnyone else want to confirm, I think this is ready to go...
Any chance of getting it into the dev release, or even into 1.0?
Comment #34
AndrewsizZ CreditAttribution: AndrewsizZ commentedWorks for me +
Comment #35
Lupin3rd CreditAttribution: Lupin3rd as a volunteer commentedWorks for me
Comment #36
skorzhStill no updates here?
Comment #37
joelpittet@korgik does #30 response to #3 mean that people can/will override menu class through the UI?
Comment #38
joelpittetInstead of "works for me" it would be nice if someone could show what they did to test this, a screenshot showing how they tested would be the easiest way to show they actually tested it.
Comment #39
joelpittetIt looks like it remove the clearfix that bartik adds. I wonder if there is away to not clobber that? Personally don't like clearfixes but would rather not have a regression.
Comment #40
skorzh@joelpittet, yes people can/will override menu class through the UI.
Bartik overrides default theme function of menu and adds clearfix there: http://cgit.drupalcode.org/drupal/tree/themes/bartik/template.php?h=7.x#...
I've used default menu class from core: http://cgit.drupalcode.org/drupal/tree/includes/menu.inc?h=7.x#n1622
Comment #41
MediaFormat CreditAttribution: MediaFormat commentedI've used this to add specific IDs and classes to menus (ie. for bootstrap, and other customizations).
IMHO there is a legitimate use case!
Comment #42
joelpittet@korgik We shouldn't be clobbering the theme's implementation of a theme hook. That will lead to support tickets as to "why current theme's theme hooks aren't firing". Can we ensure this still allows themes to override the hook? menu_attach_block does something to this effect.
@MediaFormat thanks for the screenshot of the admin. What would be really helpful would be a screenshot of the markup that changes from the admin form input. Like maybe 3 menus with different classes, one with children and one with a specific THEME_menu_tree__SUGGESTION() override so we don't break those. Something so we can cover our asses a bit.
Should we allow a hook to modify the attributes used like we do for item and link attributes?
Can we check if a theme implements this before clobbering it?
nit: Implements hook_preproces_menu_tree().
Comment #43
skorzh@joelpittet
1. To be honest I'm not sure If we should do it, because hook for item attributes calls menu_attribute_info and in this case I don't know how to call hook for menu attributes. Or can you recommend name for hook?
2. Agree.
3. Changed.
Comment #44
skorzhChanged status
Comment #45
joelpittet@korgik
hook_menu_attribute_info()
has a scope, so it would just need a new constant:MENU_ATTRIBUTES_MENU
.We can do that as a follow-up to this if you'd prefer though, but that should make it consistent.
Comment #46
mtoscano CreditAttribution: mtoscano commentedI tested the patch with both Bootstrap Tweme and Bartik themes. I can see the fields in the menu edit, but whatever I insert as css class is not reflected in the menu ul item. Basically nothing change on the front-end.
Comment #47
skorzh@mato, its because theme's hook overrides module's. Try to use #30.
Comment #48
sassafrass CreditAttribution: sassafrass as a volunteer commentedI applied the patch in #30 using the following:
curl -s -O https://www.drupal.org/files/issues/menu_attributes-ul_attributes-186204...
patch -p1 < menu_attributes-ul_attributes-1862048-30.patch
The patch applied cleanly. I was able to enter id and class into the menu attribute fields via the UI. After clearing the cache, the classes were added to the
<ul>
tag of the menu as expected.Thank-you!
*** EDIT *** Also using Menu Block module. Unfortunately, if a menu has a menu block, the class/id gets applied to the block menu rather than the menu itself. For example, I added a class to the Main Menu (id=main-menu). I use the Main Menu in a navigation region across the top of site. I also created a Main Menu block that I display in the footer of my site. This block menu gets the class, but the Main Menu in the nav region does not.
Comment #49
stephesk8s CreditAttribution: stephesk8s commented@korgik I tried #30 and am having the same results as @mato. The classes I entered are not showing on the front end.
Comment #50
MediaFormat CreditAttribution: MediaFormat commentedPatch in [#43] works for me.
@joelpittet
Here are a few screen captures in regards to [#42] :
Markup of Menu ul attributes
Markup of Menu ul attributes, with children...? (not sure this is what you wanted to test)
Markup of Menu with specific THEME_menu_tree__SUGGESTION() override
In the above case, the override impedes the module's attempt to append a class in the menu array.
Comment #51
joshuautley CreditAttribution: joshuautley commentedSame here as #46 and #49.
I tired to patch the production release and the dev release with both patch #43 and #30. Neither worked in my case.
I'm using Bootstrap Base Theme also.
I can write all the CSS to do what I want but it would save a lot of time if I didn't have to which is why I found this feature request. Thank you everyone for working on it.
Comment #52
Geijutsuka CreditAttribution: Geijutsuka commented+1 for this issue.
It would be incredibly helpful to have the ability to add ul attributes to menus.