When secondary links' source is primary links, it would be useful to know down the road (for theming purposes) which of the primary links is in the active path. All the new menu stuff rocks, and I read through this, but it appears one of my main frustrations theming primary/secondary links is still around -- namely, that parents of the active item are not marked as such.

I wrote a quick patch to deal with this, by adding a in_active_trail key to the links in the array generated by menu_navigation_links. The patch includes the addition of an in-active-trail class when theming links.

CommentFileSizeAuthor
menu_navigation_active_trail.patch1.24 KBdavideads

Comments

davideads’s picture

davideads commented
Status: Active » Needs review
birdmanx35’s picture

birdmanx35 commented
Version: 6.x-dev » 7.x-dev

This is a feature request, and feature requests go to 7.x-dev.

If this makes it easier for themers to theme, then +1!.

davideads’s picture

davideads commented
Version: 7.x-dev » 6.x-dev
Category: feature » bug
Status: Needs review » Closed (won't fix)

I filed this more than a month ago, when it certainly was not a feature request. It is a small patch, but important for a project I'm working on, and it is a not simply a feature request but a logical inconsistency that has bothered me about Drupal since I started using it in version 4.7. It could have taken 10 minutes for someone else to triage and commit. I mentioned it several times in #drupal, and a colleague of mine also tried to get someone to look at it.

I don't begrudge any FLOSS maintainer their job, and deeply respect the work done on Drupal. But we should be honest here: For the next year or two, I am now going to have to maintain a patch against all my D6 sites or recreate a lot of functionality for a tiny but crucial affordance (proper hierarchical navigation is important) and no longer have the full benefit of the community process.

Properly, this is a small bug, and we should mark it as a bug for 6.x, mark wontfix, and consider how we should approach the problem as part of the holistic D7 UI renovation project which Dries has alluded to recently.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Needs review

I think the original author won't fixed this incorrectly. Changing it back to 7.x and code needs review, since it still applies with some offsets.

@davideads: Drupal core development goes through three primary phases:

  1. "development" phase (which is what 7.x is in now), where basically any crazy patch no matter what it does, as long as it's reviewed by members of the community and found to be desirable, can get in.
  2. "code freeze" phase, during which we disallow new features and start focusing on bug fixes, so no new features are allowed, unless they are minor usability improvements.
  3. "beta/release candidate" phase (which is what 6.x was in when this patch was posted), an extension of "code freeze" when we're gearing up for a final release and are very strict about what is/is not allowed in. By this time, developers and site builders are expecting Drupal to be more or less the "real deal" they'll be working with, and as such changing things like what CSS classes are available (which will impact themes), what the interface text reads like (which impacts translations), and so on are only let in if they're deemed "critical" bugs.

Therefore, it's not that your patch is "won't fix"; it simply came too late during the 6.x development cycle to be applied.

The good news is that 7.x is in that wide-open "development" phase, so now's the perfect time to push this again. And although it's a pain to maintain a hacked version of core for this release, if this patch gets applied to 7.x, at least you'll eventually be able to do without that customization, as it'll be rolled in for you.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

patch has fuzz and should be rerolled. Seem like a basically good idea, though I'm not sure it's the best possible implementation. As a caveat, the rendering of Primary/secondary links in the page theme may change substantially for 7.

Anyhow, if you are really desperate to have this for 6.x without hacking core, you should be able to write a module to:

1) implement hook_menu, set $item['options']['alter'] to TRUE for everything in the chosen primary links menu.
2) implement hook_translated_menu_link_alter() and set a flag as you desire in $item['translated_options'] if $item['in_active_trail']

gpk’s picture

gpk commented

@pwolanin: a neat trick, albeit a bit of a fudge :-P

Presumably you mean 1) implement hook_menu_link_alter?

[Update]
@davideads: I'd suggest one tiny change: in theme_links(), use class "active-trail" instead of "in-active-trail", for consistency with http://api.drupal.org/api/function/theme_menu_item/6.

No harm in getting this into 7.x now even if there are plans for improved primary links.

Also I think the one line addition to menu_navigation_links() is suitable for inclusion in D6; theme_links() can always be re-implemented within a theme, and changing that function now could potentially foul up existing D6 themes.

gpk’s picture

gpk commented
Version: 7.x-dev » 6.x-dev
Status: Needs work » Fixed

Fixed in 6.x-dev (and 7.x-dev) http://drupal.org/node/249571.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

caktux’s picture

caktux commented

This is still happening when on an active sub-page, primary links don't get the active class and $primary_links don't carry it either. This issue is also a duplicate of http://drupal.org/node/249571. I've post a patch on 6.10 there, mostly just the menu.inc one liner.