Ticket #5662 (closed enhancement: fixed)

Opened 6 months ago

Last modified 4 months ago

Class Ancestor Pages

Reported by: AaronCampbell Assigned to: ryan
Priority: normal Milestone: 2.5
Component: General Version: 2.5
Severity: normal Keywords: has-patch tested
Cc: jeremyclarke

Description

The purpose is to add a class to ancestor pages. Right now, a CSS class is added to the current page (current_page_item), and it's parent (current_page_parent). I created a plugin (Mark Parent Pages Plugin) that uses the DOM to travel farther upstream, and tag all ancestor pages with a class as well (current_page_ancestor). Upon submitting the plugin to the plugin repository, I was told to "Please make this a core patch on Trac." So here it is. I haven't actually made the patch yet, but I'll try to when I get time. If someone else has time, please feel free. It seems like it should work just like the plugin. The only real question is whether the parent should get tagged as an ancestor as well (with both classes). The plugin does, because it's purpose was to give me collapsing menus, and doing so made my css simpler:

#nav ul li.current_page_item ul, #nav ul li.current_page_ancestor ul {

display:block;

} #nav ul li.current_page_item ul ul {

display:none;

}

instead of

#nav ul li.current_page_item ul, #nav ul li.current_page_parent ul, #nav ul li.current_page_ancestor ul {

display:block;

} #nav ul li.current_page_item ul ul {

display:none;

}

Attachments

5662.diff (1.9 kB) - added by AaronCampbell on 01/15/08 03:53:09.
5662.2.diff (2.0 kB) - added by jeremyclarke on 01/21/08 01:10:15.
fixed to not throw errors on non-page screens

Change History

01/15/08 03:53:09 changed by AaronCampbell

  • attachment 5662.diff added.

01/15/08 03:56:26 changed by AaronCampbell

  • keywords set to has-patch.

I only have 2.3.2 installed, so I can't test this patch right now on trunk, and it *is* slightly different (get_page changed). Basically, it achieves what the plugin does, but it does it in get_pages and get_post. For get_post, it uses a new function, get_post_ancestors(). Someone please test it and let me know how it works.

01/15/08 15:01:36 changed by AaronCampbell

  • keywords changed from has-patch to has-patch needs-testing.

01/15/08 19:41:52 changed by AaronCampbell

The patch is imperfect. It doesn't work if you are on the main page (ID == 0). There are two ways to fix this. First, inside get_page you can replace if ( empty($page) ) { with if ( empty($page) && $page !== 0 ) { Alternatively, we could simply use get_post rather than get_page. I guess if we plan on phasing out get_page, then we should use get_post. If get_page is supposed to stay, you could use the other fix. I suppose we could also check for null being returned from get_page.

Someone want to weigh in?

01/21/08 01:09:48 changed by jeremyclarke

  • cc set to jeremyclarke.
  • version set to 2.5.

We discussed this in irc. The problem was that the start_el function from classes.php was throwing errors if you checked $_current_page->ancestors when you weren't on a page (because the $_current_page object was empty). The solution is just to check that the get_page worked and populated $_current_page, similar to how it is done a few lines down for post_parent.

if ($_current_page && in_array($page->ID, $_current_page->ancestors)) {

the attached diff fixes it. Tested on trunk.

01/21/08 01:10:15 changed by jeremyclarke

  • attachment 5662.2.diff added.

fixed to not throw errors on non-page screens

01/21/08 01:50:07 changed by AaronCampbell

  • keywords changed from has-patch needs-testing to has-patch tested-working.

Thanks. I was over-thinking it. This was all added at Matt Mullenweg's request, so hopefully he will stop by and commit it.

01/21/08 02:11:51 changed by darkdragon

  • keywords changed from has-patch tested-working to has-patch tested.

01/21/08 03:28:18 changed by AaronCampbell

Do "tested" and "tested-working" have different meanings here? I was just following the tagging procedure I saw in another tickets:5615

01/21/08 03:29:09 changed by AaronCampbell

Sorry, link to that ticket: ticket:5615

01/21/08 04:04:42 changed by darkdragon

Yeah, tested-working has no meaning, but tested does. It is implied that when something has "tested" that whatever it was that was tested was working, or you wouldn't have used the keyword.

I'm usually fixing thee17 keywords also. Trac Keywords Use that resource on Trac Keywords.

01/21/08 04:47:57 changed by AaronCampbell

Thank you very much for the link as well as the help.

02/26/08 08:13:43 changed by ryan

  • owner changed from anonymous to ryan.

02/27/08 23:28:18 changed by ryan

  • status changed from new to closed.
  • resolution set to fixed.

(In [7074]) Introduce get_post_ancestors(). Add current_page_ancestor class to ancestors of the current page. Props AaronCampbell?. fixes #5662

02/27/08 23:29:59 changed by ryan

I removed the bits from get_pages because we've trying to get rid of those nasty loops. Split get_post_ancestors() into two functions. One to populate the ancestors field in the post object and another as a convenience function for fetching the ancestors array.