Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Please, please could you add a radio button which gives you a choice of attaching the attribute to either the <li> tag or the <a> tag - it would be fabulous
Comment | File | Size | Author |
---|---|---|---|
#51 | 0001-add-ul-attributes.patch | 4.89 KB | ijortengab |
#38 | menu_attributes_module.txt | 11.87 KB | ckt.orange |
#38 | menu_attributes_api.txt | 1.55 KB | ckt.orange |
#26 | menu_attributes-empty-fieldset-26.png | 77.77 KB | andrewmacpherson |
#25 | menu_attributes-attributes_for_li-1488960-25.patch | 7.37 KB | andrewmacpherson |
Comments
Comment #1
michaek CreditAttribution: michaek commentedI 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.
Comment #2
michaek CreditAttribution: michaek commentedI'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.
Comment #3
Danny EnglanderI 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.
Comment #4
robbdavis CreditAttribution: robbdavis commentedAnd 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?
Comment #5
Danny EnglanderOne 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.Comment #6
Jibus CreditAttribution: Jibus commentedTested #2, worked like a charm, thanks !
Comment #8
jwilson3Wondering why the patch is failing.
Comment #9
Jibus CreditAttribution: Jibus commentedThe 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
Comment #10
jwilson3But, if you read the log, it * supposedly* checks out the right branch, no?
Comment #11
Jibus CreditAttribution: Jibus commentedOh 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.
Comment #12
Jibus CreditAttribution: Jibus commented#2: menu_attributes-attributes_for_li-1488960-2.patch queued for re-testing.
Comment #13
Jibus CreditAttribution: Jibus commentedTest seems now to be ok.
Set this to RTBC, feel free to change the status if needed
Comment #14
bessone CreditAttribution: bessone commentedUsing a custom menu module, like Superfish, options on LI are not applied.
I think because Superfish rewrite all LI classes, any ideas?
Comment #15
Danny Englander@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.
Comment #16
janhendriks CreditAttribution: janhendriks commentedI'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
Comment #17
philsward CreditAttribution: philsward commentedJust tested #2 patch on 1.0-rc2 and it works great. Hope to see this incorporated into the module!
Comment #18
philsward CreditAttribution: philsward commentedOh, 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:
The only thing the older browsers DO recognize is:
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 : )
Comment #19
dwatts3624 CreditAttribution: dwatts3624 commentedJust applied the patch and can confirm that it's not working with the Superfish module.
Comment #20
jayjaydluffy CreditAttribution: jayjaydluffy commentedsubtheming 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!
Comment #21
dkingofpa CreditAttribution: dkingofpa commentedCleanly applied patch in #2 to 7.x-1.0-rc2. Moved a class from the a tag to li, worked as expected.
Comment #22
andrewmacpherson CreditAttribution: andrewmacpherson commented#2: menu_attributes-attributes_for_li-1488960-2.patch queued for re-testing.
Comment #24
andrewmacpherson CreditAttribution: andrewmacpherson commentedFollowing Commit ed7e9aa, the patch in comment #2 no longer aplies cleanly against against 7.x-1.x-dev
Re-rolling patch...
Comment #25
andrewmacpherson CreditAttribution: andrewmacpherson commentedWhoops, incorrect patch numbering.
Comment #26
andrewmacpherson CreditAttribution: andrewmacpherson commentedWhen ID, Classes and Styles are all disabled in admin/structure/menu/settings, there's an empty collapsible fieldset. Screenshot attached.
Comment #27
andrewmacpherson CreditAttribution: andrewmacpherson commentedWould 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
Comment #28
andrewmacpherson CreditAttribution: andrewmacpherson commentedItem ID, Class and Style don't get applied to to primary links, but do get applied to menu blocks.
Comment #29
renenee CreditAttribution: renenee commented#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.
Comment #30
FrancoisL CreditAttribution: FrancoisL commented#2: menu_attributes-attributes_for_li-1488960-2.patch queued for re-testing.
Comment #31
FrancoisL CreditAttribution: FrancoisL commentedHello 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
Comment #32
alexverb CreditAttribution: alexverb commentedThe 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.
Comment #33
andrewmacpherson CreditAttribution: andrewmacpherson commentedCan we clear the appropriate cache during a hook_update_N() function for users who are upgrading?
Comment #34
Aurous CreditAttribution: Aurous commentedWorks fine.
Thanks!
Comment #35
altrugon CreditAttribution: altrugon commentedPatch #25 works fine, please port it into a new release.
Thanks.
Comment #36
manuelBS CreditAttribution: manuelBS commented+1 for patch #25 to new release
Comment #37
Shane Birley CreditAttribution: Shane Birley commentedOnce caches were cleared, it seems to work well!
Comment #38
ckt.orange CreditAttribution: ckt.orange commentedI 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
Comment #39
silurius CreditAttribution: silurius commentedAnother 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?
Comment #40
Proteo CreditAttribution: Proteo commentedAnd 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.
Comment #41
drupov CreditAttribution: drupov commentedGuess what... #25 works for me too! Let's have it commited! Thanks!
Comment #41.0
drupov CreditAttribution: drupov commentedI had to change the special html characters in order for the tags in the message to read properly
Comment #42
R-H CreditAttribution: R-H commented25: menu_attributes-attributes_for_li-1488960-25.patch queued for re-testing.
Comment #43
4kant CreditAttribution: 4kant commentedthanks @andrewmacpherson for #25!
Comment #44
kopeboy CreditAttribution: kopeboy commentedCan I find the patch included in any release?
Thanks
Comment #45
joelpittetRTBC++ #25
Comment #46
bdanin CreditAttribution: bdanin commentedPatch #25 worked for me as well (after clearing all caches)
Comment #47
coolestdude1 CreditAttribution: coolestdude1 commentedPatch #25 Another confirm, working great.
Comment #48
rroblik CreditAttribution: rroblik commentedPatch #25 Another confirm, working great.
But when will it be rolled into dev branch release (or stable ?) ?
Comment #49
NancyDruI agree this needs to be committed. I would, however, prefer to see the Fieldset titles be a bit more distinguishing.
Comment #50
ijortengab CreditAttribution: ijortengab commentedthis patch from #25
new feature: ul attributes
i think we should also be able to give ul attributes
Comment #51
ijortengab CreditAttribution: ijortengab commentedSorry, 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
Comment #55
joelpittet@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?
Comment #56
NancyDruGiven 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
Comment #57
joelpittet@NancyDru I've put my hat in to co-maintain this and this patch is my main target. I'll ping @davereid again.
Comment #58
ijortengab CreditAttribution: ijortengab commented@joelpittet: :thumbs up:
i can't open a new issue, instead... create sandbox... this ... https://www.drupal.org/sandbox/m_roji28/2296939
Comment #60
joelpittetThank you all for the work on this issue! #25 has been committed to dev.
Please open a new issue for #51
Cheers
Comment #61
NancyDruThank you, Joël.