Patch to add css to allow Parent to be highlighted when on child pages
arcX - February 9, 2008 - 22:57
| Project: | Nice Menus |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs work) |
Description
So even though you clicked to go on one of the child pages, you still know that you are in such and such a section because you have the css class so you can highlight it.
| Attachment | Size |
|---|---|
| active_parent.patch | 1.54 KB |
#1
@arcX, this looks good, Thanks!
A few suggestions (new patch attached)...
$state = ($is_active) ? ' expanded' : '';). However, I've reworked it even more, so that you avoid setting the extra variable that is only used once, and so that the assignment to '' isn't even done if the condition isn't true.#2
Hey arcX, cool stuff. Thanks for getting this going. There is already an issue for this over at http://drupal.org/node/171693. Since this has movement and patches in it though, I'll dupe that one and we can keep moving here.
Also note that I am traveling for work and won't get to really look at this until mid-week.
#3
Ok, so the code was taken from that page, 171693 :) so credit is to user ivan.g. All I did was create the patch, forget where I found the code, and start using it on a live site! But it works :)
#4
Setting this to needs review.
#5
I got a failure on the last patch so I rerolled against the 5 branch. The class addition looks fine but that makes the menus look screwy in core themes since they have their own styling for expanded (notably they have a graphic which pushes things over a bit.) We will need to also edit the Nice menus default CSS to override that so it doesn't look broken out of the box.
#6
This patch needs to be redone against HEAD. I may backport to 5 but I'm not really keen on new features in 5 at this point.
#7
I am having this same problem with the module, and I cannot see how to apply this patch against the published version of the module, and I cannot find the dev version. Please could someone help me? Thanks.
#8
Dev releases can be found in the full release list (http://drupal.org/node/43047/release) or in CVS. As mentioned above the patch needs to be rerolled for Drupal 6 so it won't work at all on any D6 version. As far as I can remember the D5 release (1.2) should be pretty much the same as the dev version for 5.
#9
A patch against HEAD along with the CSS cleanup.
#10
Needs a reroll now that the JS patches have landed.
#11
subscribing
#12
Patch wasn't that bad, just fuzz. Here is a new clean patch.
#13
Hi add1sun – I may be doing something wrong, but I don't see any class applied to the parent menu item when we're on one of the child pages?
I see 'menuparent' but this was already there (just shows that it's a parent, not that we're on the page of one of it's children).
Maybe I didn't apply the patch correctly, but I just downloaded HEAD, and the patch seemed to go through without a hitch...
I'm eager to test – today ideally – so I await any advice about what I may be missing?
Cheers!
:) Scott
#14
Hrm, I'll try and get a look at it today. It should add an "expanded" class to the parent items.
#15
Cool - I'll be happy to test again - I'll keep an eye on this thread later today.
By the way, I'm testing 2 patches, but I have to test them seperately (this one, and the other patch here: http://drupal.org/node/135771 )
Earlier when I tried to patch one after the other (this one first) the otehr patch didn't work correctly – so I had to do on a new install... Is there anything I should know about patching so I can test them both together?
Thanks! – & will be back to check or updates in a few hours...
:) Scott
#16
Hm, it is adding the "expanded" class for me on a local 6.3 install. When I select Administer > Site building > Blocks, I get things like
<li id="menu-17" class="menu-path-admin-build menuparent expanded">in the parent menu items (that one is for Administer > Site building's li).#17
Oh! then I must have done something wrong ;) i will retry the patch and test...
Thanks for clarifying!
Scott
#18
test confirmed for admin menu! (on new D6.3 install patched against HEAD)... I have to run out for a few hours but will be back to test with primary links then -- thanks add1sun :) Scott
#19
Hi add1sun –
Test Failed for Primary links... I don't get the 'expanded' class on parent menu items, only something like this
<li id="menu-122" class="menu-path-node-1 menuparent ">.So works on admin menu
doesn't work on Primary links
Sorry to report this :(
Hope it helps pin down the problem a bit though?
:) Scott
#20
verified that it doesn't work on primary links. the menu_get_active_trail returns no menu trail info. No idea why right now and not sure when I'll get to actually look at it.
#21
Geeting the patch for the 'expanded' class is very important for me, but as I am not a developer :) could someone kindly tell me how to get a latest Dev version which has this class in the code? Thanks if possible.
#22
OK, I found out *why* now. #184955: menu_get_active_breadcrumb breaks breadcrumbs in primary links
So this is a core "by design" issue that I will need to work around. Thankfully stevedondley and Moonshine were working on this problem (not nice menus, but similar active trail problems) in #drupal when I happened to be there. Basically we'll need to do a menu_set_active_menu_name() before the menu_get_active_trail().
I'll play with this later this week and see if I can get a working patch.
#23
Great news! good luck with this... still on board to help test next patch ;)
#24
heh, I was actually trying to solve this problem. I got it to work! Patch coming later tonight.
#25
oh frickin sweet!
#26
Hi, sorry for the delay in getting a patch up. Here is the full theme for now. Sorry about this but I am behind in some work and so this will have to suffice until I can create a proper patch or until someone else feels the urge to touch this up and roll the patch.
It has one problem in that the 'primary-links' string is hard coded into the code. This obviously has to change. Also, how the css class is added to the code is changed from the patch above. I did not add an "expanded" class as that seemed to introduce a bug (caused a little triangle to show up next to the list item). So I just put in an "active" for all list items that are in the active path. Seemed simpler that way. It works, I tested.
<?php
function theme_nice_menu_build($menu) {
$output = '';
// Find the active trail and pull out the menus ids.
menu_set_active_menu_name('primary-links');
$trail = menu_get_active_trail('primary-links');
foreach ($trail as $item) {
$trail_ids[] = $item['mlid'];
}
foreach ($menu as $menu_item) {
$mlid = $menu_item['link']['mlid'];
// Check to see if it is a visible menu item.
if ($menu_item['link']['hidden'] == 0) {
// Build class name based on menu path
// e.g. to give each menu item individual style.
// Strip funny symbols.
$clean_path = str_replace(array('http://', '<', '>', '&', '=', '?', ':'), '', $menu_item['link']['href']);
// Convert slashes to dashes.
$clean_path = str_replace('/', '-', $clean_path);
$class = 'menu-path-'. $clean_path;
$class .= in_array($mlid, $trail_ids) ? ' active' : '';
// If it has children build a nice little tree under it.
if ((!empty($menu_item['link']['has_children'])) && (!empty($menu_item['below']))) {
// Keep passing children into the function 'til we get them all.
$children = theme('nice_menu_build', $menu_item['below']);
// Set the class to parent only of children are displayed.
$class .= $children ? ' menuparent ' : '';
// Add an expanded class for items in the menu trail.
$output .= '<li id="menu-'. $mlid .'" class="'. $class .'">'. theme('menu_item_link', $menu_item['link']);
// Build the child UL only if children are displayed for the user.
if ($children) {
$output .= '<ul>';
$output .= $children;
$output .= "</ul>\n";
}
$output .= "</li>\n";
}
else {
$output .= '<li id="menu-'. $mlid .'" class="'. $class .'">'. theme('menu_item_link', $menu_item['link']) .'</li>'."\n";
}
}
}
return $output;
}
?>
#27
Hey thanks Steve! I'll get a patch rolled later today or tomorrow that addresses the menu name coming in dynamically.
Re: the class name, my earlier patch has the CSS correction needed to work with the expanded class. I'm still not sure if that is better or not. I think that the way core uses these is where "expanded" is for a parent in use and "active" is used for the menu item of the page itself when on that page. In those definitions the active trail should be using "expanded" and not "active." The other option, of course, is to simply create a new class to distinguish it altogether. I don't know if it is better to use core classes that folks may already have themed or just add our own new one. Opinions?
#28
Bleh, i couldn't help poking at this and basically this doesn't work if you have more than one nice menu. The first nice menu name gets set as active and then that's it. It is a static var in menu_set_active_menu_name(). If we can't figure out how to "reset" the active menu name each time it is called then we may just need to write our own helper function to get the trail.
I've attached a patch of what we have so far.
#29
Thanks for this. I have applied this patch yesterday. I found that some further changes are still needed.
1. The line in the patch which was
$class .= $children ? 'menuparent ' : '';works better for me if it is$class .= $children ? ' menuparent' : '';2. The line (a bit above that from point 1)
$children = theme('nice_menu_build', $menu_item['below']);should take in the newly added function parameter, i.e. it should be$children = theme('nice_menu_build', $menu_item['below'], $menu_name);#30
Thanks vitalie, I'll reroll it after I commit the menu depth patch (which will happen soon).
#31
Thanks, finally got it working..
Though, for performance I suggest
$trails_idbe set static.#32
This patch is exactly what we have been waiting for! But since installing it with the two extra patches of the patch described by Vitalie above, drop downs don't work in IE7 at all. They are fine in Ffx. We have had a skilled CSS person look at it, and he thinks there must be an issue with the code, but we are stumped. Any ideas? I opened this as a separate issue at http://drupal.org/node/292483 but mention here in case it helps. Thanks for any advice...
#33
Reroll with the depth patch in HEAD now and adding vitalie's changes. Untested. The patch definitely isn't optimized yet, but unless we can figure out the multiple menu issue, this patch isn't going in anyway. Just to be clear *this is not a fully working patch* and I am not sure this direction will work in the long run. I am more and more inclined to write a helper function that does this for us manually, since the API is not designed to do what we are trying to do here.
I don't know what about this would make things stop working in IE7 and this hasn't happened to me.
#34
Hi add1sun!
just making sure I understand where the progress is at with this issue...
Thanks for clarifying :)
Scott
#35
Hey Scott, here is the run-down:
The depth patch has been committed to 6.x-2.x-dev (HEAD). So #33 is the only one that will apply cleanly now.
#33 should work for *one* nice menu. If you only use one nice menu, then it should be fine. If you use more than one, only one of the menus (the first one called by NM) will get the expanded class for its items. So, you can use it for now, but it is unacceptable to be committed to the module long term as is. Any solution that goes in will need to work will all NM blocks. I'm not sure that core API will accommodate that so we may need to write our own helper function to determine the trails.
If you only need one *specific* menu block to work for the time being you can use the patch and manually set the name of the menu you want to use. You can do that a number of ways but the simplest is to probably add a line in function theme_nice_menu_build() along with the patch.
In the function, before the line:
+ // Find the active trail and pull out the menus ids.+ $active_menu = menu_set_active_menu_name($menu_name);
you can add:
$menu_name = 'primary-links';Where primary-links can be changed to any menu name, which you can find in the
menu_namecolumn of themenu_customtable in the DB.I will not be able to work on this any more until September, unless I manage to sit down with it while at DrupalCon Szeged. I'll be very busy there though, so I doubt much progress will be made until I get back.
#36
Just to say: we have got the patched, patched version to work (expanded class) in our dev site with seven nice menus, with no problem. But we still have not fixed the issue of drop downs not working in IE7! No doubt something with our theme/css because it works ok with Garland enabled.
#37
Hey add1sun – great, thanks for clarifying...
(hope u have a great time at Drupalcon!)
edward.peters – what do u mean by 'patched, patched version'? It'd be great if I could do with more than one menu... :)
#38
@scottrigby: see comment #29 above by my developer Vitalie.
#39
Just to be clear, Vitalie's additions are now in patch #33, so that patch is complete.
@edward.peters, it doesn't work for me with multiple menus. that is to say that Nice menus itself works fine but the expanded class is only added to one of the menus and not to the others. Can you verify that multiple menus get the class and, if so, please provide a fresh patch with your local changes.
#40
Yes, I think we (see comments #36 and #38) do not actually ever display two nice menus at the same time.
It seems to me that the problem with multiple menus is coming from
menu_set_active_trail()function (called by the respective _get_ function) which has a static$trailand when once generated (in the longest else clause in that function) never changes unless one specifically gives it another trail (but where to get it from in the first place, if not just from a replicated code?!). Practically, once a trail is set, it remains the same. This is why on the second and third, etc nice menus we are getting actually the trail from the first one. Should not this core function provide a way for us to tell it we want a new trail to be generated?A possible round-about solution follows, but let me first say that I think one more change is needed to the patch in comment #33. In the function
theme_nice_menu_tree()just above the one we are talking about here, at the end (line 300, i think), the line$output['content'] .= theme('nice_menu_build', $menu, $depth);should be$output['content'] .= theme('nice_menu_build', $menu, $depth, $menu_name);(so with the $menu_name passed on).The possible round about: just add this at the beginning of the theme_nice_menu_build function:
function theme_nice_menu_build($menu, $depth = -1, $menu_name = NULL) {
static $once;
if (!isset($once)) { /* maybe the current item is in a second or third nice menu, so set its trail first! */
$real_menu_name = db_result(db_query("SELECT menu_name from {menu_links} where link_path='%s'", $_GET['q']));
if ($real_menu_name) {
$active_menu = menu_set_active_menu_name($real_menu_name);
$trail = menu_get_active_trail($active_menu);
}
$once = 1;
}
//and then the $output = ''; follows (just as it is now)
#41
I had the same issue. The patch 33 worked for me.
Furthermore I added another functionality. I wanted to have the active menu point also highlighted. I got a working solution with the following code:
<?php
function theme_nice_menu_build($menu, $depth = -1, $menu_name = NULL) {
$output = '';
// Find the active trail and pull out the menus ids.
$active_menu = menu_set_active_menu_name($menu_name);
$trail = menu_get_active_trail($active_menu);
foreach ($trail as $item) {
$trail_ids[] = $item['mlid'];
}
$count = 1;
foreach ($menu as $menu_item) {
$mlid = $menu_item['link']['mlid'];
// Check to see if it is a visible menu item.
if ($menu_item['link']['hidden'] == 0) {
// Build class name based on menu path
// e.g. to give each menu item individual style.
// Strip funny symbols.
$clean_path = str_replace(array('http://', '<', '>', '&', '=', '?', ':'), '', $menu_item['link']['href']);
// Convert slashes to dashes.
$clean_path = str_replace('/', '-', $clean_path);
$class = 'menu-path-'. $clean_path;
//Check if menu-item is active active
if (((drupal_get_normal_path($menu_item['link']['href']) == $_GET['q']) || (drupal_is_front_page() && $menu_item['link']['href'] == '<front>')) && $count == 1) {
$class .= ' expanded';
// Prevent duplication of id for html validity.
$count++;
}
// If it has children build a nice little tree under it.
if ((!empty($menu_item['link']['has_children'])) && (!empty($menu_item['below'])) && $depth != 0) {
// Keep passing children into the function 'til we get them all.
$children = theme('nice_menu_build', $menu_item['below'], $depth, $menu_name);
// Set the class to parent only of children are displayed.
$class .= $children ? ' menuparent ' : '';
// Add an expanded class for items in the menu trail.
$class .= in_array($mlid, $trail_ids) ? ' expanded' : '';
$output .= '<li id="menu-'. $mlid .'" class="'. $class .'">'. theme('menu_item_link', $menu_item['link']);
// Check our depth parameters.
if ($menu_item['link']['depth'] <= $depth || $depth == -1) {
// Build the child UL only if children are displayed for the user.
if ($children) {
$output .= '<ul>';
$output .= $children;
$output .= "</ul>\n";
}
}
$output .= "</li>\n";
}
else {
$output .= '<li id="menu-'. $mlid .'" class="'. $class .'">'. theme('menu_item_link', $menu_item['link']) .'</li>'."\n";
}
}
}
return $output;
}
?>
#42
Hi, does this patch work for Drupal 5.x.1 versions?
#43
@dcalderon, no it doesn't and this feature will not be added to the D5 version unless someone else writes a backport patch for it.
#44
what about the first version posted by ivan.g? (we've it here: http://drupal.org/node/171693#comment-599475 ) wasn't it working for D5? Maybe I can figure out how to make it a backport patch later... actually, i thought what douggreen upload in comment 1: http://drupal.org/node/219804#comment-723769 was a patch for D5, but i then read the following comments and they were all related to D6.
#45
Hey, add1sun, thank you for pointing me here!
Once I got the css rule overrides figured out, the latest function from brainski works well for me on 6.4 - at least on nodes that are directly referenced by the menu.
You can see it working at http://atma.syn8.com/content/test-page - created placeholder submenus for testing nested menus, even though our intended heirarchy is only one deep.
I'll keep an eye on this thread to see if I can help further with testing any new revisions.
#46
More on the subject of highlighting of indirect nodes:
We've been using http://drupal.org/project/menutrails to set rules for how menu .active classes get applied based on content type and/or taxonomy.
This had the highly desirable result that our taxonomy-based primary and secondary menus highlighted appropriately to show the context of nodes accessed indirectly via taxonomy or views.
You can see this operational on the secondary menu at: http://atma.syn8.com/media-forum/articles/brief-history-media
Think there's a way for Nice Menus to support Menu Trails inserting the .active class like it does on the core menus? Or maybe there's a better way to get the same result?