Regression: restore primary/secondary link classes.

It's not critical. I agree, those classes are nasty but it can be overridden with theme functions.

Code freeze in effect so marking for 7.x-dev. Might be able to go into 6.x-dev if there's a *simple* solution to this.

Whatever ideas you have, spill it out right here or it'll get ignored.

thanks

This is extremely important: Drupal 6 is bringing support for 'Pure CSS themes,' and our menu structures are one of the remaining 'impossible to cleanly style' portions of the system.

Anything we can do to improve the situation is a huge boost -- we should NOT require people to override theme functions (especially ones as nested and convoluted as the menu theme functions) to get cleanly styled menus.

Ben Mauer, output looks nice but we need a solution to get there. I'll try to look at it. I remember playing with it and had to do a bunch of string manipulations to get there. Might not be good for core.

Might have to dig beyond the theme function to clean up.

And yeah, I agree Eaton. Since we have a chance now, lets clean it up.

here's part of it. http://drupal.org/node/103391

Removes the redundant classes. Needs reviewing.

I got the other part pretty much figured out but there's an issue with primary links. http://drupal.org/node/164394

Once that is fixed, a parent > child class could be created for even more flexibility.

We could do something like this for now. menu_secondary_links() is pretty much the same.

function menu_primary_links() {
  $tree = menu_tree_page_data('primary-links');
  $links = array();
  foreach ($tree as $item) {
    $l = $item['link']['options'];
    $l['href'] = $item['link']['href'];
    $l['title'] = $item['link']['title'];
    $links['menu-'. $item['link']['mlid']] = $l; // Added a key so theme_links converts to classes.
  }
  return $links;
}

Right now in D6, no keys are set making classes of 0, 1, 2, etc..

Having it increment like that is useless since the classes won't match up to what your trying to select as you add more links.

Tagging. I support this effort and will see what I can add.

The removal of the redundant classes created from theme_links was commited but this patch pretty much brings us back to what we had.

Uses the menu id (mlid)? to feed into theme_links as classes. I guess it'd be idea to have them as html id's but that would involve a lot more.

More appropriate title..

Small problem with this. Since "menu_navigation_links()" doesn't pass an active state anymore, it depends on theme_links() to do it. Works well except in one case. Setting "" as the menu link will not trigger the active class.

<front> for the menu link is what I meant.

subscribe

I can confirm that this patch applies (1 line offset) and fixes the problem. I don't know that I'm wild about using menu-$mlid as a class, though. That feels very hacky (since it's a class bound to the specific menu item), but since the array key needs to be unique I don't see a away around that.

Adding that value to the ID, too, however, is one more line of code. The attached patch does so.

Unless someone has a better solution to the class question, I'll call this RTBC.

The patch is blank.

Have I mentioned how much I HATE Eclipse?

I'm not sure if the keyed menu classes are all that useful, or why not having them would be critical.

We get designs from our designers all the time that have every single menu item having a different color. Like, 3-4 this year alone, and that's just the sites I've worked on. Not being able to uniquely reference a menu item makes that extremely difficult. In fact, it would be even better if all menu items, including those in blocks, had a unique class or id, but that's out of the scope of this patch.

Relative CSS rules are well and good, until you need to start referencing "first child", "second child", "third child", etc. Then you need either explicit, unique classes/ids or you need to pretend IE doesn't exist. :-)

Committed to CVS HEAD. Thanks.

The css id added in this issue is invalid (it shouldn't start with a number) as reported here: http://drupal.org/node/179486

The attached patch simply removes the id since it is seems to be redundant (nearly identical to the class). Alternatively it could be prefixed with 'menu-' (making it identical to the class).

Grr. That's my fault from when I redid the rerolled patch with the ID in it. I'd actually favor putting the ID in, since it should be unique. The class is only specified because the $links array requires it. Unless someone can point out where using an ID would cause the ID to appear twice on a page, I'd say fix the ID rather than removing it.

Do we really need the ids at this point? It would be nicer to have them as id's but then we need both classes and id's due to theme_links.

Maybe we can include this back in and rework through id's.

http://api.drupal.org/api/function/theme_menu_links/5

It was seldom used but I don't think it should have been removed in d6.

I think it's important that we establish some consistency here. The .active class gets placed on a tags in every other instance in drupal. Now as a selector, it's good to have the active class on the list item as well, but I think having it on the anchor will make designer's lives much easier, and it'll make the implementation in drupal more consistent. Plus, I really do think this would be more semantic in any case. I don't have time to put up code, but I thought I'd mention this for discussion.

Eclipse

I asked my theming partner and she hemed and hawed for a bit and then said to just use the class, we don't need the ID. Attached patch is just a reroll to get rid of offsets and CRs, and is just a one liner to get rid of the ID on the element. Let's squeeze this in. (Marking critical since it generates invalid HTML right now.)

The titles shouldn't be printed if there's no value. Also, in the case of having a lone primary link, it could be troublesome to add both the first and last classes at once. I'm not sure if that's a new or old thing though.

Btw, can someone explain what active-trail stands for?

I committed Crell's patch from #22. If there are other issues in this code, it can still be discussed, but it is probably not a fault of changes in this issue, is it? (Mark this fixed if the discussion continues in another issue).

The patches from this issue have now been committed, so it is now fixed.

Please open a new issue for further discussion of possible future changes.