Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#12812 closed enhancement (fixed)

New Nav Menu needs to have ancestor class

Reported by: joostdevalk's profile joostdevalk Owned by: filosofo's profile filosofo
Milestone: 3.0 Priority: normal
Severity: blocker Version: 3.0
Component: Menus Keywords: has-patch dev-feedback
Focuses: Cc:

Description

In the new menu functionality, the only thing that's missing from the "old" wp_list_page method is the parent / ancestor class: I'd like to highlight a menu item even if that menu item's page is only the ancestor page of the current page, to highlight the current section.

To do this, we have to simply match the menu item against the current post->ancestors array. Patch attached.

Attachments (8)

page-parent.diff (902 bytes) - added by joostdevalk 14 years ago.
Patch v1
ancestor-patch.diff (1.0 KB) - added by joostdevalk 14 years ago.
Patch v2
12812.refresh.diff (1.0 KB) - added by xibe 14 years ago.
Patch refresh.
nav-menu-parent-classes.12812.diff (5.4 KB) - added by filosofo 14 years ago.
add-ancestor-classes.12812.diff (1.9 KB) - added by filosofo 14 years ago.
notice-and-styling-frontend-menus.12812.diff (2.2 KB) - added by filosofo 14 years ago.
menu_bug.html (2.9 KB) - added by ryanhellyer 14 years ago.
HTML showing ancestor/parent bugs
current-menu-ancestor-actual.12812.diff (2.4 KB) - added by filosofo 14 years ago.

Download all attachments as: .zip

Change History (49)

@joostdevalk
14 years ago

Patch v1

#1 @Denis-de-Bernardy
14 years ago

post ancestors isn't always set (there's a ticket on this with that name, more or less)

#2 @joostdevalk
14 years ago

I've got another patch, which changes the class to menu-item-ancestor and adds menu-item-parent as well. If post ancestors isn't always set, that should be fixed too :)

@joostdevalk
14 years ago

Patch v2

#3 @ryan
14 years ago

Instead of using global $post, let's try $wp_query->get_queried_object().

#4 follow-ups: @ryan
14 years ago

Is that the right place to put that code? Should it be in Walker_Nav_Menu?

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

#6 @daltonrooney
14 years ago

I am running 3.0 beta 1 and I found this feature missing as well. I applied the patch and it works as expected. I hope this makes it into the final release.

#7 @daltonrooney
14 years ago

This new code works for me on pages and single posts, but throws an error on the blog homepage.

The error I get is

Warning: array_values() [function.array-values]: The argument should be an array in /home/dalton/public_html/nk/wp-includes/nav-menu.php on line 227

#8 @momo360modena
14 years ago

  • Cc momo360modena added

#9 @ocean90
14 years ago

  • Component changed from General to Menus
  • Keywords needs-refresh added; has-patch dev-feedback needs-testing removed
  • Owner set to filosofo

@xibe
14 years ago

Patch refresh.

#10 @xibe
14 years ago

  • Keywords has-patch dev-feedback needs-testing added; needs-refresh removed

#11 @filosofo
14 years ago

I think the proposed patch is the wrong way to do this. wp_setup_nav_menu_item decorates a menu object. If you think of menus in terms of MVC such objects are part of the model, not the view (and it's the view that handles things like context-specific HTML classes).

I'm not just objecting out of academic concern for architectural purity or good software design. wp_setup_nav_menu_item has a clear, discrete purpose which is independent of global state. Trying to account for global state here almost guarantees bugs or hacks to get around bugs in the future.

#12 in reply to: ↑ 4 @gvvaughan
14 years ago

  • Cc gvvaughan added

Replying to ryan:

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

Agreed. The patch just happens to work when upgrading from a blog which previously built a menu from the page hierarchy, provided that you build a new wp_nav_menu hierarchy that matches.

I installed the patch after doing just that, and it appears to do the right thing. BUT after going into the Edit Pages admin page and setting all pages to 'No Parent', the patches in this ticket are no longer able to decorate menu items with the correct (or any in fact) menu-item-parent or menu-item-ancestor classes.

I don't know how to get a list of the children of the current menu item, otherwise I would have provided a correct patch... if someone can enlighten me on that detail, I will write the patch though.

#13 @aldolat
14 years ago

  • Cc aldolat@… added

#14 @nacin
14 years ago

  • Keywords needs-patch added; has-patch dev-feedback needs-testing removed

#15 @kpdesign
14 years ago

  • Cc kpdesign added

I ran into this issue also on a site I'm developing using 3.0 and the new nav menu functionality. After searching the wp-testers list, I found an email from Andrew Nacin that talked about this same issue and mentioned an open Trac ticket for it.

Would like to see this resolved; unfortunately I'm not able to provide a patch to fix it. :(

#16 in reply to: ↑ 4 @husobj
14 years ago

Replying to ryan:

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

Ryan, you have a point there - should a menu ancestor be highlighted if the current selected menu item is a child of that menu item (but not necessarily a child of that page)?

... and should the menu item be highlighted if the current page is a child page of that ancestor (but not necessarily a child menu item)

My opinion is that a menu item should be highlighted if the current page is a child of that page OR a child menu item of that page.

Just my 2 cents :)

#17 @ryan
14 years ago

Doing the post parent and category parent instead of the real menu parent is better than nothing. We definitely need to get Home working.

#18 @filosofo
14 years ago

I'll make a patch to add parentage classes.

#19 @ryan
14 years ago

  • Severity changed from normal to blocker

Going to bump this up to blocker because it's an important piece of menu functionality that would be really nice to have in 3.0.

#20 @filosofo
14 years ago

  • Keywords has-patch added; needs-patch removed
  • Version set to 3.0

nav-menu-parent-classes.12812.diff sets the classes property of menu item objects, so that each menu item indicates via class name when a menu item is the parent of the currently-queried thing: it indicates both when it's the parent of that thing's corresponding menu item and when it corresponds to the real parent of the thing.

  • current-menu-item: A class value of the menu item when it corresponds to the currently-requested page.
  • current-menu-parent: A class value of the menu item when it is the parent of the currently-requested page.
  • current-[category, page, post, etc.]-parent: A class value of the menu item when the object to which it corresponds is the parent of the currently-requested object.

#21 @ryan
14 years ago

(In [14876]) Add parent classes to nav menu. Props filosofo. see #12812

#22 follow-up: @ryan
14 years ago

Do we want to add current_page_ancestor, current_page_item, and current_page_parent for back compat? I think we'd only apply them to actual pages and home.

#23 in reply to: ↑ 22 @joostdevalk
14 years ago

Replying to ryan:

Do we want to add current_page_ancestor, current_page_item, and current_page_parent for back compat? I think we'd only apply them to actual pages and home.

Might be smart and save us some crying in the forums and blogs :)

#24 @ryan
14 years ago

Does twentyten have any current menu item styling?

#25 @ryan
14 years ago

We might also want to add the new ids to the page walker so that theme authors can target the new selectors and still have them work when wp_page_menu() is used as a fallback.

#26 @filosofo
14 years ago

add-ancestor-classes.12812.diff adds the ancestors.

If you decide to hack in current_page_ancestor (underscores), it could be done in the Walker class with str substitution.

#27 @ryan
14 years ago

(In [14881]) Add ancestor classes. Props filosofo. see #12812

#28 @ryan
14 years ago

(In [14882]) Active menu styling. Props ianddtewart. see #12812

#29 @ryan
14 years ago

(In [14900]) Back compat menu classes. see #12812 #13379

#30 @ryan
14 years ago

This warning is generated when clicking a category link:

PHP Notice:  Undefined property: stdClass::$post_type in /.../wp-includes/nav-menu-template.php on line 364

And this one for the same line when clicking a custom link:

PHP Notice:  Trying to get property of non-object in /.../wp-includes/nav-menu-template.php on line 364

#31 @ryan
14 years ago

  • Resolution set to fixed
  • Status changed from new to closed

@ryanhellyer
14 years ago

HTML showing ancestor/parent bugs

#32 follow-up: @ryanhellyer
14 years ago

This does not appear to be fixed, in fact I think it might be worse than before. The HTML attached, is a chunk of code from my test install running WP 3.0 beta 3.

To create that code, I set a Page to be at the fourth level, the list item gets classes of "current-menu-item" and "current_page_item", which makes complete sense.

However ... the parent of that Page, regardless of whether the parent is in fact it's "Page parent", is given the class of "current_page_parent" as well as the expected "current-menu-parent". Since it isn't actually the current pages parent and is just a parent in the menu, it shouldn't be given the "current_page_parent" class IMO.

Then it gets really bizarre, as the actual top level page ancestor, regardless of where it is in the menu structure, is given a class of "current-menu-ancestor" despite not being an ancestor within the menu structure and also classes of "current-page-parent" (expected) and "current_page_ancestor" (unexpected).

Unfortunately I don't have a solution :( I'm just pointing out the problem in the hope that someone with more knowledge than I can help figure out a way to make it work!

Thanks to Diana for bringing this to my attention :)

#33 in reply to: ↑ 32 @filosofo
14 years ago

Replying to ryanhellyer:

However ... the parent of that Page, regardless of whether the parent is in fact it's "Page parent", is given the class of "current_page_parent" as well as the expected "current-menu-parent". Since it isn't actually the current pages parent and is just a parent in the menu, it shouldn't be given the "current_page_parent" class IMO.

This was done deliberately to be backwards-compatible with wp_page_menu styling.

Then it gets really bizarre, as the actual top level page ancestor, regardless of where it is in the menu structure, is given a class of "current-menu-ancestor" despite not being an ancestor within the menu structure

That does sounds like a bug. I'll investigate.

and also classes of "current-page-parent" (expected) and "current_page_ancestor" (unexpected).

Why is current_page_ancestor unexpected? Isn't it the current page's ancestor?

#34 follow-up: @ryanhellyer
14 years ago

Why is current_page_ancestor unexpected? Isn't it the current page's ancestor?

Because in this particular case it is the immediate parent of the current page and therefore is the page parent, not the page ancestor.

#35 @nacin
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#36 in reply to: ↑ 34 @filosofo
14 years ago

Replying to ryanhellyer:

Because in this particular case it is the immediate parent of the current page and therefore is the page parent, not the page ancestor.

That's both the meaning of "ancestor" and WP's prior terminology with pages and categories: a parent is an ancestor.

#37 @nacin
14 years ago

"That does sounds like a bug. I'll investigate." filosofo->reviewing?

#38 @ocean90
14 years ago

  • Keywords has-patch removed

I get this view http://grab.by/4Vqn. It happens if I had an item twice and for example I click the first 'Tags A and C' link. The HTML output:

<ul id="menu-testmenu" class="menu">
<li id="menu-item-4" class="menu-item menu-item-type-custom"><a href="http://localhost/wp/">Home</a></li> 
<li id="menu-item-402" class="menu-item menu-item-type-post_type current-menu-item"><a href="http://localhost/wp/blog/archives/166">Tags A and C</a></li> 
<li id="menu-item-403" class="menu-item menu-item-type-post_type current-menu-parent"><a href="http://localhost/wp/blog/archives/162">Tag B</a> 
  <ul class="sub-menu"> 
    <li id="menu-item-413" class="menu-item menu-item-type-post_type current-menu-item"><a href="http://localhost/wp/blog/archives/166">Tags A and C</a></li> 
  </ul> 
</li> 
</ul>

Perhaps ryanhellyer means this.

#39 @nacin
14 years ago

ocean90: Nothing we can do about that; we don't know which menu item was clicked.

#40 @ocean90
14 years ago

  • Keywords has-patch dev-feedback added

Last patch works good for me. Screenshot: http://grab.by/4VUD
The active menu is 'page with comments'. Without the patch the item 'Tab b' will not be highlighted.

#41 @ryan
14 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [15257]) Menu ancestor fixes. Props filosofo. fixes #12812

Note: See TracTickets for help on using tickets.