Hey there,

Overriding the theme_menu_link causes problems, the attached (sorry, way too busy for a patch) is a modified version which uses theme_link instead.

(I also checked the dev version and it was no different, the theme_menu_link function has to be removed altogether.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mehrpadin’s picture

Sorry, something went wrong, here's it again.

pfx’s picture

I try this new special_menu_items.module and now nolink work fine with the superfish menu.
Thanks

pfx’s picture

I just noticed that the tag <a is effectively replaced by the tag <span but there is no class nolink in it.

a.ross’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
9.66 KB

This is a step in the right direction to fix the issues with the module generating a faulty link. It should be optional how menu items are formatted (span, href="#", ...) but this is the way to go. I've merged in this file to the latest revision of the module and have attached the resulting patch. I think development continued in the master branch, so the patch should be applied there, but I'm not entirely sure.
Everything appears to be working fine with the patch, including the menu item listing in admin/structure/menu/manage/<menu>. I'll mark this RTBC, any issues that are caused by the mismanagement of code in git shouldn't need to affect this issue.

hermes_costell’s picture

If this helps with the module steamrolling special classes I'm trying to send to the individual menu items that would be stellar!

a.ross’s picture

Do you mean something like this?

kingfisher64’s picture

applying the patch on #4 to the special menu items mod works great. If you want the styling to match the default superfish theme styling (in terms of margin & padding) add the following to the relevant stylesheet.

ul.menu li {
	margin: 0;
}
	
.sf-menu li {
	padding: 0.75em 1em;
}
	
.sf-menu.sf-style-default a {
	padding: 0;
	border-left: none;
	border-top: none;
}
Boobaa’s picture

The patch in #4 works great; without that this module is pretty much useless: redirecting the user to a 404 page from a "nolink" is a PITA.

DuaelFr’s picture

Status: Reviewed & tested by the community » Needs work

Hi mehrpadin and a.ross and thank you very much for your work.

I took a look to your code and patch and I cannot leave this issue as RTBC because :

  1. you cannot mark your own patch as RTBC since it is an open door to mistakes :/
  2. the patch file is a bit messy with confusing things like this (16 times)
    -<?php 
    +<?php
  3. the patch seems to include a lot more than what is covered by this issue. Think about maintainers who will have to judge if this have to be commited. You may open separate issues.

Regards

a.ross’s picture

Yeah, patch was made on a windows system, which causes problems with line endings and possibly other stuff. I didn't actually mark my patch as RTBC per se, I meant to mark Mehrpadin's code RTBC. I've reviewed and tested it and we're using it in production right now.

I'll see if I can have a second look at it at home (where I'm using a proper OS).

gagarine’s picture

I will love if someone can propose a patch against the branch 7.x-1.0. #1221294: option to use <a href="#"> for "nolink" items to not break other modules is going to have an incidence on this (patch 23 was commited)?

gagarine’s picture

Assigned: mehrpadin » Unassigned
gagarine’s picture

Proper patch against 7.x.1.0. But need one against dev.

tars16’s picture

#13 failed for me with 7.x-1.0:

patch < override-theme_link-instead-the-theme_menu_link-1447988-13.patch
patching file special_menu_items.module
Hunk #1 FAILED at 41.
Hunk #4 FAILED at 153.
2 out of 4 hunks FAILED -- saving rejects to file special_menu_items.module.rej
a.ross’s picture

I can confirm.

pog21’s picture

Same problem here as #14. Any progress on this?

a.ross’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
4.62 KB

Patch rerolled against -dev. Please review

a.ross’s picture

New patch, this time using the 'proper' name for the default menu_link function. This also removes the theme_menu override completely.

gagarine’s picture

Status: Needs review » Fixed

Commited. Thanks a lot

pog21’s picture

Thanks for the patch. It seems to be working. One small thing, isn't there supposed to be a class='nolink' added to the span?

gagarine’s picture

@pog21 please feel free to open a new bug

Status: Fixed » Closed (fixed)

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