Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Exploratus’s picture

I need that also.

rocbrook’s picture

+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.

meecect’s picture

+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.

gusans’s picture

+1

This would be great for using with bootstrap nav classes.

jdcollier’s picture

This would be wonderful!

Nilard’s picture

+1

batigol’s picture

Not 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!

bdanin’s picture

Issue summary: View changes

This 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.

bdanin’s picture

is this a duplicate of https://drupal.org/node/1488960 ?

jwilson3’s picture

@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.

bdanin’s picture

It 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.

batigol’s picture

It would be very helpful in D7 as well. Default classes like menuitem1, menuitem2, menuitem3 + active, first, last etc. is a must in any cms.

bdanin’s picture

Here'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.

jwilson3’s picture

@#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.

joelpittet’s picture

skorzh’s picture

Version: 7.x-1.0-rc2 » 7.x-1.x-dev
Status: Active » Needs review
FileSize
3.48 KB

I'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.

batigol’s picture

@korgin, thanks! I'm gona test it next weekhend.

skorzh’s picture

Still no updates here ;(

batigol’s picture

@korgik, sorry forget to give a review - everything works very well!

Spleshka’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @korgik! Patch itself looks good. I also have tested it at simplytest.me, and it works as expected. Nice job!

AndrewsizZ’s picture

Hey,
Exactly what I need and works!

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

This 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

  1. +++ b/menu_attributes.install
    @@ -28,6 +28,7 @@ function menu_attributes_uninstall() {
    +  variable_del("menu_attributes_menus");
    

    single quotes.

  2. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
    +  $attrs = variable_get('menu_attributes_menus', array());
    

    Please don't use the short version of the name. $attrs => $attributes.

  3. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
    +    '#title' => 'ID',
    ...
    +    '#title' => 'Classes',
    

    Shouldn't these be translated as well?

  4. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
    +    '#default_value' => $default_attrs['id'],
    ...
    +    '#default_value' => $default_attrs['class'],
    

    attrs vs attributes again.

  5. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
    +    'class' => $values['attributes']['class']
    

    Ending comma on on last array item.

  6. +++ b/menu_attributes.module
    @@ -351,3 +405,34 @@ function menu_attributes_preprocess_menu_link(&$variables) {
    + * Pre-preprocess function for template_preprocess_menu_tree().
    

    "Prepares variables for theme_menu_tree()."
    @see https://www.drupal.org/node/1913208

  7. +++ b/menu_attributes.module
    @@ -351,3 +405,34 @@ function menu_attributes_preprocess_menu_link(&$variables) {
    + * Adds menu name to $vars. Gets menu name from first link item.
    ...
    +function menu_attributes_prepreprocess_menu_tree(&$vars) {
    

    Use $variables instead of $vars.

  8. +++ b/menu_attributes.module
    @@ -351,3 +405,34 @@ function menu_attributes_preprocess_menu_link(&$variables) {
    + * Override theme_menu_tree().
    + *
    + * Output menu attributes.
    

    Implements instead of Override. and doesn't need the description under because all theme functions output markup.

skorzh’s picture

Assigned: Unassigned » skorzh

Thank you for review, I will fix

skorzh’s picture

Assigned: skorzh » Unassigned
Status: Needs work » Needs review
FileSize
3.48 KB
3.45 KB

New patch and interdiff attached.
Pay please attention, I've added array_filter() to menu_attributes_menu_tree() in order to remove empty array items.

MediaFormat’s picture

Great 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

skorzh’s picture

Assigned: Unassigned » skorzh
Status: Needs review » Needs work

Thanks for feedback, yeah, seems you are right, I will check it

joelpittet’s picture

This 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 #25

skorzh’s picture

Assigned: skorzh » Unassigned
Status: Needs work » Needs review
FileSize
3.59 KB
1.35 KB

D8 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.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/menu_attributes.module
    @@ -421,6 +421,7 @@
    +  $variables['depth'] = $first_element['#original_link']['depth'];
    

    Nice trick! Depth context provided!

  2. +++ b/menu_attributes.module
    @@ -428,7 +429,10 @@
    +  $attributes = array('class' => 'menu');
    

    Class's values needs to be an array (true for d8 and d7)
    Should be:
    $attributes = array('class' => array('menu'));

  3. +++ b/menu_attributes.module
    @@ -428,7 +429,10 @@
    +    $attributes = array_filter($menus[$variables['menu_name']]);
    

    To keep with existing styles should you merge the 'menu' class on there as well?

skorzh’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
1.52 KB

3. 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).

MediaFormat’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch #1862048-30: Class attribute for entire menu,

Works well.

Thank you @korgik

skorzh’s picture

No updates here?

MediaFormat’s picture

Anyone 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?

AndrewsizZ’s picture

Works for me +

Lupin3rd’s picture

Works for me

skorzh’s picture

Still no updates here?

joelpittet’s picture

@korgik does #30 response to #3 mean that people can/will override menu class through the UI?

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

Instead 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.

joelpittet’s picture

It 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.

skorzh’s picture

@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

MediaFormat’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
FileSize
9.8 KB

I've used this to add specific IDs and classes to menus (ie. for bootstrap, and other customizations).
menu-attributes

IMHO there is a legitimate use case!

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

@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.

  1. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
    +  $form['attributes']['class'] = array(
    

    Should we allow a hook to modify the attributes used like we do for item and link attributes?

  2. +++ b/menu_attributes.module
    @@ -132,6 +132,60 @@ function menu_attributes_get_menu_attribute_info() {
      * @see menu_edit_item()
    
    @@ -351,3 +405,34 @@ function menu_attributes_preprocess_menu_link(&$variables) {
    +  $registry['menu_tree']['function'] = 'menu_attributes_menu_tree';
    

    Can we check if a theme implements this before clobbering it?

  3. +++ b/menu_attributes.module
    @@ -351,3 +405,34 @@ function menu_attributes_preprocess_menu_link(&$variables) {
    + * Prepares variables for theme_menu_tree().
    

    nit: Implements hook_preproces_menu_tree().

skorzh’s picture

@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.

skorzh’s picture

Status: Needs work » Needs review

Changed status

joelpittet’s picture

@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.

mtoscano’s picture

I 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.

skorzh’s picture

@mato, its because theme's hook overrides module's. Try to use #30.

sassafrass’s picture

I 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.

stephesk8s’s picture

@korgik I tried #30 and am having the same results as @mato. The classes I entered are not showing on the front end.

MediaFormat’s picture

Patch 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.

joshuautley’s picture

Same 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.

Geijutsuka’s picture

+1 for this issue.
It would be incredibly helpful to have the ability to add ul attributes to menus.