Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michaek’s picture

I agree that it's important to be able to set attributes on the list item. However, several of the attributes can only be applied to an anchor: target, relationship, access key. It probably makes more sense to provide a separate fieldset for list item attributes.

michaek’s picture

Title: <li> <a> » Attributes for LI element
FileSize
40.81 KB
7.41 KB

I'm providing a patch here that adds a 'scope' key for attribute info definitions, allowing attributes to be used for anchors, list items, or both. A fragment of the UI is represented in the attached screenshot. Sensible defaults are provided for the existing attributes.

I wasn't able to add tests supporting this feature, because I couldn't get the tests to run at all. If a maintainer is able to run the tests, perhaps they could add a few to provide coverage.

Danny Englander’s picture

I just tested the patch and it works great, exactly what I needed. This is really great for theming an entire menu trail and only having to enter a class in the UI once so it's a bit more extensible. Thank you.

robbdavis’s picture

And my first patch ever...WORKED! Thanks so much @michaek !

Can anyone share how I know if this patch gets applied to the menu attributes module? In other words, what's best practice for dealing with a patched module? How do I make sure it doesn't get updated without the patch?

Danny Englander’s picture

One thing I just noticed yesterday while theming a new design for a client with this patch is that the new features (class, id etc.. added to the menu's <li> tag) did not work with the Drupal Superfish module whereas custom classes added to a menu <a> tag did.

To test, I went back to using the main menu from the theme settings and sure enough the <li> tag's custom classes I added showed up. I know that's probably an edge case anyway. In the end, I got rid of the Superfish module and used the Menu Block Module to render my entire main menu tree and then manually added in the Superfish script and this worked great, all my custom classes were still there that I added to the <li> tag.

Jibus’s picture

Status: Active » Needs review

Tested #2, worked like a charm, thanks !

Status: Needs review » Needs work

The last submitted patch, menu_attributes-attributes_for_li-1488960-2.patch, failed testing.

jwilson3’s picture

Version: 7.x-1.0-rc1 » 7.x-1.x-dev
Status: Needs work » Needs review

Wondering why the patch is failing.

Jibus’s picture

The patch is tested on the master branch, which contains only three files: menu_attributes.info menu_attributes.install menu_attributes.module.

The file patch the file menu_attributes.api.php which is not present, therefore the test fail.

Also, i think that menu_attributes.test should be updated according to the new feature provided by the patch of Michaek

jwilson3’s picture

But, if you read the log, it * supposedly* checks out the right branch, no?

...
 Main branch [7.x] checkout [complete].
...
Dependency branch [7.x-1.0-rc1] checkout [complete].
...
git apply --check -p1 /var/lib/drupaltestbot/sites/default/files/review/menu_attributes-attributes_for_li-1488960-2.patch 2>&1] failed
Jibus’s picture

Oh yes you're right, but i see also:

Command [git clone 'git://git.drupal.org/project/menu_attributes.git' '/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/menu_attributes' --reference /var/cache/git/reference 2>&1] succeeded.

I cloned this repository, and it contains only three files.

Jibus’s picture

Jibus’s picture

Status: Needs review » Reviewed & tested by the community

Test seems now to be ok.

Set this to RTBC, feel free to change the status if needed

bessone’s picture

Using a custom menu module, like Superfish, options on LI are not applied.

I think because Superfish rewrite all LI classes, any ideas?

Danny Englander’s picture

@H-BES - see my comment above (#5) in regard to the Drupal Superfish module (if that's what you are using). I don't think it's an issue with Menu attributes, it's Superfish most likely causing the problems.

janhendriks’s picture

I've patched the module, all ok. But it doesn't seem to do anything. I can add a class and ID to the menu item but they don't show up in the source code. Anyone any thoughts on this?

ID: list-item_1
Classes: active

<div id="main-menu" class="navigation">
	<h2 class="element-invisible">Hoofdmenu</h2>
	<ul id="main-menu-links" class="links clearfix">
		<li class="menu-843 first"><a href="/~deb64479/nl/inschrijven" title="Subscribe" id="menu_2">Inschrijven</a></li>
		<li class="menu-782"><a href="/~deb64479/nl/tourinformatie" title="Information" id="menu_3" class="active-trail">Tourinfo</a></li>
		<li class="menu-772"><a href="/~deb64479/nl/nieuws" id="menu_4">News</a></li>
		<li class="menu-818"><a href="/~deb64479/nl/fotos" title="Photo's" id="menu_5">Foto's</a></li>
		<li class="menu-885 last"><a href="/~deb64479/nl/geschiedenis" id="menu_6">Geschiedenis</a></li>
	</ul>      
</div>
philsward’s picture

Just tested #2 patch on 1.0-rc2 and it works great. Hope to see this incorporated into the module!

philsward’s picture

Oh, I will also mention that from my experience, applying the CSS to the li for hover events, is the only way you can keep from breaking older browsers. This is another reason to make sure this ability is included.

Older browsers don't recognize stuff like:

a.menu-01:hover

The only thing the older browsers DO recognize is:

li.menu-01 a:hover

So, if the class is applied to the anchor, you can't do on hover stuff... I haven't messed with the active class it, but I would think it would make it difficult too since that class is applied to the anchor itself.

Thx for the #2 patch! I'd be pulling my hair out otherwise : )

dwatts3624’s picture

Just applied the patch and can confirm that it's not working with the Superfish module.

jayjaydluffy’s picture

subtheming AdaptiveThemes and applied the #2 patch against 7.x-1.0-rc2 and 7.x-1.x-dev and can confirm it's NOT WORKING. switched to Bartik and still NOT WORKING. pls advise. Thanks!

dkingofpa’s picture

Cleanly applied patch in #2 to 7.x-1.0-rc2. Moved a class from the a tag to li, worked as expected.

test@prod01:~/domains/test.com/current/sites/all/modules/contrib/menu_attributes$ wget http://drupal.org/files/menu_attributes-attributes_for_li-1488960-2.patch
--2013-04-09 13:33:35--  http://drupal.org/files/menu_attributes-attributes_for_li-1488960-2.patch
Resolving drupal.org... 140.211.10.16
Connecting to drupal.org|140.211.10.16|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 7588 (7.4K) [text/plain]
Saving to: `menu_attributes-attributes_for_li-1488960-2.patch'

100%[===================================================================================================>] 7,588       --.-K/s   in 0.05s   

2013-04-09 13:33:35 (160 KB/s) - `menu_attributes-attributes_for_li-1488960-2.patch' saved [7588/7588]

test@prod01:~/domains/test.com/current/sites/all/modules/contrib/menu_attributes$ git apply -v menu_attributes-attributes_for_li-1488960-2.patch 
Checking patch menu_attributes.api.php...
Checking patch menu_attributes.module...
Applied patch menu_attributes.api.php cleanly.
Applied patch menu_attributes.module cleanly.
test@prod01:~/domains/test.com/current/sites/all/modules/contrib/menu_attributes$
andrewmacpherson’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, menu_attributes-attributes_for_li-1488960-2.patch, failed testing.

andrewmacpherson’s picture

Status: Needs work » Needs review
FileSize
7.37 KB

Following Commit ed7e9aa, the patch in comment #2 no longer aplies cleanly against against 7.x-1.x-dev

Re-rolling patch...

andrewmacpherson’s picture

Whoops, incorrect patch numbering.

andrewmacpherson’s picture

Status: Needs review » Needs work
FileSize
77.77 KB

When ID, Classes and Styles are all disabled in admin/structure/menu/settings, there's an empty collapsible fieldset. Screenshot attached.

andrewmacpherson’s picture

Would be good to update the options in admin/structure/menu/settings, so that ID, Class and Style can be enabled for either/both the LI and A elements.

e.g. under Class, have two check boxes: Enable class attribute for Menu Item, and Enable class for Menu Link

andrewmacpherson’s picture

Item ID, Class and Style don't get applied to to primary links, but do get applied to menu blocks.

renenee’s picture

#25 worked great for me. Thank you so much andrewmacpherson! Also, I'd like to note that I had to clear all caches to get it to work, in case anyone has issues.

FrancoisL’s picture

Status: Needs work » Needs review
FrancoisL’s picture

Hello I applied #25 but it seems to not working.

New fields appeared on the admin side but no callses added for the li tags in menu.

I tried other solutions (b adding code in template.php and of course the patch in this page but nothing appeared.
I'm also using JQuery Menu, do you think there can be a problem with this module?

Thanks in advance

alexverb’s picture

The patch is working for me. Just don't forget to clear your caches because its a new preprocess function that's added to the module trough the patch.

andrewmacpherson’s picture

Can we clear the appropriate cache during a hook_update_N() function for users who are upgrading?

Aurous’s picture

Works fine.

Thanks!

altrugon’s picture

Status: Needs review » Reviewed & tested by the community

Patch #25 works fine, please port it into a new release.

Thanks.

manuelBS’s picture

+1 for patch #25 to new release

Shane Birley’s picture

Once caches were cleared, it seems to work well!

ckt.orange’s picture

I patched as #25.
But still can't assign a class/id to the li tag of the menu.

Here's attache the module and api file patched.
Thank you

silurius’s picture

Another confirmation that #25 works. Thank you, Andrew!

ckt.orange, are you sure you cleared *all* caches after patching #25? (Go to /admin/config/development/performance and click "Clear all caches".) Or maybe you mistook the new "Menu item attributes" collapsible fieldset that the patch creates for the standard "Menu link attributes" collapsible fieldset?

Proteo’s picture

And yet another confirmation that patch from #25 works great, even using PostgreSQL, many thanks @andrewmacpherson. If you're having problems with it, make sure you're using the DEV version, and remember to clear caches.

drupov’s picture

Guess what... #25 works for me too! Let's have it commited! Thanks!

drupov’s picture

Issue summary: View changes

I had to change the special html characters in order for the tags in the message to read properly

R-H’s picture

4kant’s picture

thanks @andrewmacpherson for #25!

kopeboy’s picture

Issue summary: View changes

Can I find the patch included in any release?
Thanks

joelpittet’s picture

RTBC++ #25

bdanin’s picture

Patch #25 worked for me as well (after clearing all caches)

coolestdude1’s picture

Patch #25 Another confirm, working great.

rroblik’s picture

Patch #25 Another confirm, working great.

But when will it be rolled into dev branch release (or stable ?) ?

NancyDru’s picture

I agree this needs to be committed. I would, however, prefer to see the Fieldset titles be a bit more distinguishing.

ijortengab’s picture

this patch from #25
new feature: ul attributes
i think we should also be able to give ul attributes

ijortengab’s picture

FileSize
4.89 KB

Sorry, the patch number #50 had an error on the menu with many branches.
Use a replacement patch on this comment.
----------------------
This patch from #25
New feature: ul attributes
i think we should also be able to give ul attributes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 0001-add-ul-attributes.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: 0001-add-ul-attributes.patch, failed testing.

joelpittet’s picture

@ijortengab this issue is great for lots of reasons. And your contribution does add another nice bit to it, but in order to get this patch in it may be prudent to keep it as light as possible and not add extra features at the moment. Though I would encourage you open a followup issue with this patch and link the two issues together. So reviewers and maintainers can keep the patch/issue noise down to a minimum and more likely for it to get into the module. Is that ok for you?

NancyDru’s picture

Given that the last commit was well over a year ago, I have my doubts that it will get committed in any form.

Dealing with unsupported (abandoned) projects

joelpittet’s picture

@NancyDru I've put my hat in to co-maintain this and this patch is my main target. I'll ping @davereid again.

ijortengab’s picture

@joelpittet: :thumbs up:
i can't open a new issue, instead... create sandbox... this ... https://www.drupal.org/sandbox/m_roji28/2296939

joelpittet’s picture

Status: Needs work » Fixed

Thank you all for the work on this issue! #25 has been committed to dev.

Please open a new issue for #51

Cheers

NancyDru’s picture

Thank you, Joël.

Status: Fixed » Closed (fixed)

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