Reviewed & tested by the community
Project:
Menu Minipanels
Version:
7.x-2.x-dev
Component:
Miscellaneous
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
27 Aug 2012 at 14:22 UTC
Updated:
2 Nov 2016 at 10:50 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
capellicI'm seeing this, too. I'm also having this problem when hovering over the block title when using the Menu Block Module. See attached for illustrations
I upgraded from rc7 this morning and I can't say I noticed this issue in rc7.
Comment #2
capellicI can confirm that rc7 DOES NOT have a problem with showing the menu on breadcrumbs.
I can confirm that rc7 DOES have a problem with showing the menu on the menu block title.
Comment #3
damienmckennarc7 didn't support having the primary & secondary navigation set to the same menus, rc8 does - this might be inadvertently tripping it up.
Comment #4
capellicI need to revise my observations about #2 above. rc7 does have a problem with showing the menu on the breadcrumbs but on in certain situations.
No megamenu attached to "About Us":
Home > About Us > Node Title
Megamenu IS attached to "About Us":
Home > About Us > Level 2 > Node Title
Comment #5
capellicApologies... I was wrong in both #2 and #4 above. RC7 works perfectly-- the Menu Block title link does NOT show the Menu Minipanel and the same goes for the breadcrumb. Rolling back to RC7 wasn't a problem as there were no database updates.
Comment #6
damienmckennaI have a version that will work correctly and avoid adding menus to the breadcrumb items (see #1764048: NEED USER FEEDBACK: How do you use Menu_MiniPanels with D7?), but it's comes with a major architecture change. I'll see about uploading it as v2 in the next few days.
Comment #7
capellicThanks, Damien for giving this your attention. You're a busy man! For now, I've rolled back to RC7.
Comment #8
damienmckennaWould you mind testing rc9? Thanks.
Comment #9
JBecker commentedHi Damien. After updating to rc9, I still have the problem that the minipanel appears in the breadcrump and in the menu block.
Comment #10
damienmckennaI couldn't reproduce this with the current -dev version.
Comment #11
Kendall Totten commentedI can confirm this problem is still happening with Menu Minipanels 7.x-1.1.
http://screencast.com/t/Crcov0zVI
Comment #12
damienmckennaThis doesn't happen in my local D7 sandbox, so am going to poke at it for a bit.
Comment #13
damienmckennaOk, it *does* happen in my local sandbox if I don't limit my testing to a link for the homepage. Gah.
Comment #14
damienmckennaThis is a dirty, nasty, horrible way of doing it, but it works. It probably has some performance problems too.
Comment #15
paulmckibbenThe patch applies cleanly for me (on 7.x-1.1) and solves the problem.
Comment #16
damienmckennaI'm concerned about committing this due to it loading debug_backtrace() for every enabled link, that's bound to add overhead :-|
Comment #17
damienmckennaFYI a long-term fix, though major architectural change, is being worked on: #1902378: Rewrite output generation to work off template_preprocess_page()
Comment #18
original_ZX81 commentedQuick and dirty jQuery removal we used:
$('#yourbreadcrumbid .menu-minipanel').removeClass('menu-minipalel').qtip("destroy");
Comment #19
alex.bukach commenteddigitalphasedesign, your solution works, but you mistyped class in
.removeClass().Comment #20
dshields commented@digitalphasedesign or @Alex Bukach,
I wonder if you could tell me where to place that single line of code that solves this issue.
Thanks a lot!
Comment #21
alex.bukach commented@dshields, in the case js should be added through theme, since CSS selector for breadcrumb container may differ from theme to theme. You can create custom
foo.jsfile, then go to your theme.infofile add a line there:scripts[] = foo.js. The content offoo.jsfile can be as follows:Please note that you should replace
#yourbreadcrumbidwith CSS selector of your theme's breadcrumb container. This is disadvantage of the solution above, that it solves the issue for particular theme inside the theme, not inside the module itself.The details regarding working with js files in Drupal can be found at http://drupal.org/node/171213.
Comment #22
dshields commentedThanks a million!
Comment #23
Tyler the Creator commented#21 fix almost works. It still throws a javascript error when mousing over the breadcrumb link in question.
Uncaught TypeError: Cannot read property 'options' of undefinedComment #24
davidd07 commentedAnother fix is to use hook_menu_breadcrumb_alter() this is called from menu_get_active_breadcrumb() which creates a new instance of our menu. Which means we are free to do what we want with our breadcrumb without effecting our actual menu items.
Comment #25
fredfab commentedThis solution (#24) seems perfect ! Thanks you so much davidd07. I was waiting with the RC7 version until this fix.
YOURTHEME> Mythemename)No more "menu mini panel" on breadcrumb's links.
Comment #26
jenlamptonI can reproduce this problem consistently with taxonomy. If you create a nested taxonomy structure, and then base a menu off your terms using taxonomy_menu or similar, you see the menu mini panel on all child taxonomy pages - in the breadcrumb.
Both solutions #21 and #24 (and even #14) remove the panel from the breadcrumb, but wouldn't it be better if we were able to prevent it from being added there in the first place?
Instead of overriding theme_link() (which has performance implications of it's own) can't we add the menu via theme_links or something that doesn't affect every l() function site-wide, including the breadcrumb?
Attached is a patch that takes this new approach to adding mini panels this way instead.
Comment #28
jenlamptonOne more time without a dpm() and with better docs updates.
Comment #29
jenlamptontest bot.
Comment #31
jwilson3This comment suggests that I can still implement theme_links in my theme without causing havoc, which would be true for new sites assuming a themer knows (eg, via README) to copy from *our* version of the function and not from core. However, any site that has a theme that already implements theme_links in their theme will break if they update the module with this patch, therefore, I'm not sure how we could get this into the 7.x-1.x branch without the potential for breaking sites.
The point about performance is a fair one I suppose, since core goes out of its way to tweak performance of the l() function to not use theme_link(). So, I'm thinking this could be fair game for 7.x-2.x, but to be frank, this would change such an integral part of how the 7.x-2.x version works, its going to require a bit of testing across the gamut of contrib modules that implement menu solutions (nice menus, dhtml_menu, and superfish being the primary ones).
Why limit this to menu items that have an 'href', would it not be better to just check for the $link['minipanel'] here?
Comment #32
jwilson3test again against dev branch.
Comment #33
jwilson328: menu_minipanels-breadcrumb-menu-render-fix-1758866-28.patch queued for re-testing.
Comment #35
jwilson3Bah, I see now that patch in #28 *is* actually against 7.x-2.x!
Comment #36
jwilson328: menu_minipanels-breadcrumb-menu-render-fix-1758866-28.patch queued for re-testing.
Comment #38
jwilson326: menu_minipanels-breadcrumb-menu-render-fix-1758866-26.patch queued for re-testing.
Comment #39
jwilson3prolly needs a reroll.
Comment #40
cs_shadow commentedRerolled the patch in #28 against the latest HEAD.
Comment #41
jwilson3Great. It's green now, the code looks fine except for some dangling whitespace on a blank line.
What this needs are two things:
* manual testing for the breadcrumb and/or taxonomy menu module.
* performance profiling before and after.
Comment #42
loopduplicate commented#40 doesn't work for me; when I apply it, the mini panel doesn't appear in the breadcrumb or in the menu. Because I have to get it working tonight, I used the approach from #13. Here's an updated patch for that approach.
Comment #43
codi commentedFor anyone landing here and having a problem like loopduplicate where all minipanels go missing. Make sure you're printing the main menu links through the page template and not through something like context or menu_block. The patch provided by jenlampton uses theme_links which doesn't get run by those modules so you won't see your minipanels get attached.
Comment #44
loopduplicate commentedThanks codi #43. You are right, I am using menu_block. Thanks for the tip.
Cheers,
Jeff
Comment #45
tom friedhof commentedThe patch in #40 works good. Here is another reroll of #40 that also gets rid of a php notice.
Comment #46
damienmckennaI think we'd need to spin off separate issues to add support for menu_block, but I'm (again) hugely grateful for jenlampton for providing a key architectural fix. I'm using v2 on a rather large site at the moment, I'll try to give the patch a review soon.
Comment #47
damienmckennaProbably also needs to be rerolled for 7.x-1.x and 6.x-1.x, given I just committed #2213851: Performance consideration for sites with large theme registry.
Comment #48
damienmckennaComment #50
damienmckennaFYI a certain multisite sports organization in the US has been using the patch in #45 for a few months.
Comment #51
damienmckennaThis patch is for the v7.x-1.x branch, though needs testing.
Comment #52
damienmckennaComment #53
damienmckennaThe last patch for 7.x-1.x was incorrect, this one should work though.
Comment #55
davidsheart02 commentedI've tried the patch in #53. Once applied, attempting to clear the cache throws errors like:
Commenting out the Link function in the return statement on line 306 allowed the function to run without throwing the errors. Obviously, the menu minipanel didn't work with the link function commented out.
Comment #56
jastraat commentedI would vote for not including the patch for #45 (or its predecessors) in the 7.x-2.x branch. While more elegant, the new code broke all our site implementations.
We're using a Superfish menu block to piggy-back off of the JS available with that module and support both cascading and mega menu drop-downs in the same menu.
I imagine there are a number of folks using Superfish, Menu Block, Panels, Context or something similar for menus and committing this patch would break the module for them all.
While not nearly as elegant or performant, I chose to add a custom theme function as outlined in #24.
Comment #57
jastraat commentedFor individuals interested in using the patch in #45 (which is elegant) but using menus that don't go through theme_links(), here is an example solution that we implemented to make the module compatible with Superfish.
Make sure 'Use a theme function for hyperlinks' and 'Use a theme function for menu items.' are enabled in your Superfish block (which they are by default).
Then add a custom theme function in your custom theme that overrides theme_superfish_menu_item_link() and appends the menu mini-panel. As an example:
Comment #58
b-prod commentedThose patches (#45, #53) are not compatible with menu block...
Comment #59
b-prod commentedThis issue is still there in the current 2.x version.
Here is a patch that fixes the problem and should keep compatible with other contributed modules.
Comment #61
b-prod commentedThe tests result is No valid test... Not sure of what it means, but the patch does not break other features.
Comment #63
anybodyRequed tests. The problem sadly still exists. :(
Comment #65
anybodyThere are simply no tests defined. For me the patch works well manually tested and applied against 7.x-2.x-dev. RTBC from me. Would you agree?
Comment #66
b-prod commentedThe patch is used on PROD on some website fir some time and works great.
Comment #67
anybodyComment #68
damienmckennaI'm sorry for being an absentee maintainer.. I committed the patch #53 locally to both branches but then realized it would break backwards compatibility, as reported #56, so didn't push the changes.
FYI the tests fail because there are no tests in the module.
What I think would be the best way of handling this would be to write tests to confirm the output works.
Comment #69
b-prod commentedWriting tests means a lot of time to spend, because there are no tests at all... so it seems this issue will remain opened for a long long time.
As D8 is the new way to go, why bother on creating test suites for a version that is to be deprecated? It would be really better to focus on test suites for D8, no?