Comments

capellic’s picture

StatusFileSize
new71.66 KB

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

capellic’s picture

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

damienmckenna’s picture

rc7 didn't support having the primary & secondary navigation set to the same menus, rc8 does - this might be inadvertently tripping it up.

capellic’s picture

I 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

capellic’s picture

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

damienmckenna’s picture

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

capellic’s picture

Thanks, Damien for giving this your attention. You're a busy man! For now, I've rolled back to RC7.

damienmckenna’s picture

Status: Active » Postponed (maintainer needs more info)

Would you mind testing rc9? Thanks.

JBecker’s picture

Hi Damien. After updating to rc9, I still have the problem that the minipanel appears in the breadcrump and in the menu block.

damienmckenna’s picture

Version: 7.x-1.0-rc8 » 7.x-1.x-dev
Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

I couldn't reproduce this with the current -dev version.

Kendall Totten’s picture

Status: Closed (cannot reproduce) » Active

I can confirm this problem is still happening with Menu Minipanels 7.x-1.1.
http://screencast.com/t/Crcov0zVI

damienmckenna’s picture

This doesn't happen in my local D7 sandbox, so am going to poke at it for a bit.

damienmckenna’s picture

Ok, it *does* happen in my local sandbox if I don't limit my testing to a link for the homepage. Gah.

damienmckenna’s picture

Status: Active » Needs review
StatusFileSize
new1.44 KB

This is a dirty, nasty, horrible way of doing it, but it works. It probably has some performance problems too.

paulmckibben’s picture

The patch applies cleanly for me (on 7.x-1.1) and solves the problem.

damienmckenna’s picture

I'm concerned about committing this due to it loading debug_backtrace() for every enabled link, that's bound to add overhead :-|

damienmckenna’s picture

FYI a long-term fix, though major architectural change, is being worked on: #1902378: Rewrite output generation to work off template_preprocess_page()

original_ZX81’s picture

Quick and dirty jQuery removal we used:

$('#yourbreadcrumbid .menu-minipanel').removeClass('menu-minipalel').qtip("destroy");

alex.bukach’s picture

digitalphasedesign, your solution works, but you mistyped class in .removeClass().

dshields’s picture

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

alex.bukach’s picture

@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.js file, then go to your theme .info file add a line there: scripts[] = foo.js. The content of foo.js file can be as follows:

jQuery(document).ready(function($) {
  $('#yourbreadcrumbid .menu-minipanel').removeClass('menu-minipanel').qtip("destroy");
}); 

Please note that you should replace #yourbreadcrumbid with 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.

dshields’s picture

Thanks a million!

Tyler the Creator’s picture

#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 undefined

davidd07’s picture

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

/**
 * Implements hook_menu_breadcrumb_alter().
 * Fix: Make sure the menu_minipanels module does not attach the qTips function
 * to any breadcrumb links by removing the minipanel 'flag' from menu items.
 * See issue: https://drupal.org/node/1758866
 */
function YOURTHEME_menu_breadcrumb_alter(&$active_trail, $item){
  foreach ($active_trail as $key => $menu_item) {
    if (isset($active_trail[$key]['localized_options']['minipanel']) && !empty($active_trail[$key]['localized_options']['minipanel'])) {
      unset($active_trail[$key]['localized_options']['minipanel']);
    }
  }
}
fredfab’s picture

Version: 7.x-1.x-dev » 7.x-1.1

This solution (#24) seems perfect ! Thanks you so much davidd07. I was waiting with the RC7 version until this fix.

  • Update the module from RC7 to 7.X-1.1
  • Paste your code in template.php file (YOURTHEME > Mythemename)
  • Clear the cache and that's it !

No more "menu mini panel" on breadcrumb's links.

jenlampton’s picture

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

Status: Needs review » Needs work

The last submitted patch, 26: menu_minipanels-breadcrumb-menu-render-fix-1758866-26.patch, failed testing.

jenlampton’s picture

One more time without a dpm() and with better docs updates.

jenlampton’s picture

Status: Needs work » Needs review

test bot.

Status: Needs review » Needs work

The last submitted patch, 28: menu_minipanels-breadcrumb-menu-render-fix-1758866-28.patch, failed testing.

jwilson3’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
+/**
+ * Replacement theme function for theme_links().
+ *
+ * Injects menu minipanels into to links without disturbing themes that may
+ * also implement the theme_links() function.
+ *
+ * @see menu_minipanels_theme_registry_alter()
+ * @see theme_links()
+ */
+function menu_minipanels_theme_links($variables) {

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

+      if (isset($link['href'])) {
+        // Pass in $link as $options, they share the same keys.
+        $output .= menu_minipanels_theme_link($link);
+      }

Why limit this to menu items that have an 'href', would it not be better to just check for the $link['minipanel'] here?

jwilson3’s picture

Status: Needs work » Needs review

test again against dev branch.

jwilson3’s picture

Status: Needs review » Needs work

The last submitted patch, 28: menu_minipanels-breadcrumb-menu-render-fix-1758866-28.patch, failed testing.

jwilson3’s picture

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

Bah, I see now that patch in #28 *is* actually against 7.x-2.x!

jwilson3’s picture

Status: Needs review » Needs work

The last submitted patch, 28: menu_minipanels-breadcrumb-menu-render-fix-1758866-28.patch, failed testing.

jwilson3’s picture

jwilson3’s picture

prolly needs a reroll.

cs_shadow’s picture

Status: Needs work » Needs review
StatusFileSize
new5.38 KB

Rerolled the patch in #28 against the latest HEAD.

jwilson3’s picture

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

loopduplicate’s picture

StatusFileSize
new2.36 KB

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

codi’s picture

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

loopduplicate’s picture

Thanks codi #43. You are right, I am using menu_block. Thanks for the tip.
Cheers,
Jeff

tom friedhof’s picture

The patch in #40 works good. Here is another reroll of #40 that also gets rid of a php notice.

damienmckenna’s picture

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

damienmckenna’s picture

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

damienmckenna’s picture

damienmckenna’s picture

FYI a certain multisite sports organization in the US has been using the patch in #45 for a few months.

damienmckenna’s picture

StatusFileSize
new5.8 KB

This patch is for the v7.x-1.x branch, though needs testing.

damienmckenna’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
damienmckenna’s picture

StatusFileSize
new5.87 KB

The last patch for 7.x-1.x was incorrect, this one should work though.

The last submitted patch, 51: menu_minipanels-n1758866-51.patch, failed testing.

davidsheart02’s picture

I've tried the patch in #53. Once applied, attempting to clear the cache throws errors like:

PHP Fatal error: Allowed memory size of 1073741824 bytes exhausted (tried to allocate 72 bytes) in /my/project/root/includes/theme.inc on line 265

Fatal error: Allowed memory size of 1073741824 bytes exhausted (tried to allocate 64 bytes) in /my/project/root/sites/all/modules/contrib/menu_minipanels/menu_minipanels.module on line 306

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.

jastraat’s picture

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

jastraat’s picture

For 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:

function THEME_superfish_menu_item_link($variables) {
  $minipanel = '';
  $menu_item = $variables['menu_item'];
  $link_options = $variables['link_options'];
  if (module_exists('menu_minipanels')) {
    if (isset($link_options['minipanel']) && !empty($link_options['minipanel']) && $menu_item['link']['has_children'] == FALSE) {
      $link_options['attributes']['class'][] = 'menu-minipanel';
      $link_options['attributes']['class'][] = 'menu-minipanel-' . $link_options['menu_minipanels_hover']['mlid'];
      if (!in_array('menuparent', $link_options['attributes']['class'])) {
        $link_options['attributes']['class'][] = 'menuparent';
      }
      $minipanel = theme('menu_minipanel', array('minipanel_name' => $link_options['minipanel'], 'mlid' => $link_options['menu_minipanels_hover']['mlid']));
    }
  }
  return l($menu_item['link']['title'], $menu_item['link']['href'], $link_options) . $minipanel;
}
b-prod’s picture

Those patches (#45, #53) are not compatible with menu block...

b-prod’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
StatusFileSize
new1.69 KB

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

Status: Needs review » Needs work

The last submitted patch, 59: menu_minipanels-breadcrumb-issue-1758866-59.patch, failed testing.

b-prod’s picture

Status: Needs work » Needs review

The tests result is No valid test... Not sure of what it means, but the patch does not break other features.

Status: Needs review » Needs work

The last submitted patch, 59: menu_minipanels-breadcrumb-issue-1758866-59.patch, failed testing.

anybody’s picture

Requed tests. The problem sadly still exists. :(

The last submitted patch, 59: menu_minipanels-breadcrumb-issue-1758866-59.patch, failed testing.

anybody’s picture

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

b-prod’s picture

The patch is used on PROD on some website fir some time and works great.

anybody’s picture

Status: Needs work » Reviewed & tested by the community
damienmckenna’s picture

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

b-prod’s picture

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