The menu system UI works today! Congrats, compared to my last foray :P

Now, it seems like functionality has been removed compared to Drupal 5:
* ability to set primary and secondary links to arbitrary menus
* ability to set primary and secondary links to the same thing so that secondary links are just the second level of what is under the selected primary
* ability to set "default menu for content" to show all menus

I don't think any of these features should go away.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Patch please.

kkaefer’s picture

Status: Active » Needs review
FileSize
6.57 KB
  • Implemented the selection of primary and secondary links
  • If primary links == secondary links, the secondary links now display the second level of primary tabs

I did not add the “Show all menus” item because at the moment, it does not really make sense to select anything here.
Users can’t create menu items otf when they don’t have “administer menu” and if they have, they can see all menus anyway.

kkaefer’s picture

Changes as per pwolanin on IRC: Don’t modify menu_tree_page_data() and implement the logic in menu_primary_links() and menu_secondary_links().

kkaefer’s picture

Cleaned up patch with pwolanin's help. Added some documentation.

kkaefer’s picture

Loop is more legible now.

Dries’s picture

Not being able to cache the navigation block is not a show-stopper for this patch and is something we could work on in a follow-up patch. The only way to solve this is by providing a setting in the navigation block's configuration page, or to create a contributed module that alters the cache-field in the blocks table.
That or you could edit one line of code. Let's focus on what we have now, and then consider another iteration of improvements. I don't think these improvements will change the API -- they'd merely add configuration settings.

(mis-posted)

pwolanin’s picture

per conversation with Dries - make this a theme setting, not a menu module setting.

pwolanin’s picture

note, the patch also includes the single-line patch that is here: http://drupal.org/node/166491 so that the global theme settings are useable.

kkaefer’s picture

Changed some strings. Functionality-wise it works correct.

merlinofchaos’s picture

IMO, these are not decisions to be made by the theme layer.

Everything we're talking about is choosing what to present: Choosing which menu should be primary and secondary, etc. That is not the presentation layer, that is logic.

The theme layer should only choose HOW to present the data it's given. It should not be selecting data sources. The biggest choice about the data it should be making is whether or not to present data it is given, and how to present it.

This should remain a menu setting.

pwolanin’s picture

Per Merlin's suggestion, as compared to the previous patch, the only setting as to *which* content to use is done in menu module. The theme system has options only to toggle the primary and secondary links on or off.

pwolanin’s picture

fix typo and re-roll for this commit: http://drupal.org/node/166491

pwolanin’s picture

Title: Implement features from Drupal 5 menu system » Let Secondary links be children of Primary as in the Drupal 5 menu system

make the title reflect the patch.

kkaefer’s picture

Status: Needs review » Reviewed & tested by the community

Still applies, tested it successfully. This is RTBC, we need this in D6.

Gábor Hojtsy’s picture

Is it a current/suggested practice to use "(Optional)" no optional parameters, in parenthesis and not as a leading "Optional ..." word in the sentence?

pwolanin’s picture

re-rolled without parentheses around Optional.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Ok, looks fine. Committed, thanks!

Jaza’s picture

Title: Let Secondary links be children of Primary as in the Drupal 5 menu system » Let Primary and Secondary links be arbitrary menus as in the Drupal 5 menu system
Status: Fixed » Needs work

Errr... what happened to "ability to set primary and secondary links to arbitrary menus"? That was in the first few versions of this patch, but it seems to have disappeared around #12. Could we put that back, please?

We should also remove the new "Source for the secondary links" option. How did that sneak in? I don't see any discussion about it in this issue. Anyway, it's not as flexible as actually setting the menu for the secondary links, so I see it as a step backward. Let's get rid of it, and put the old options back, which weren't so limiting.

Jaza’s picture

I also created http://drupal.org/node/170322 (having 'proper' menus for books) which is related to this issue.

pwolanin’s picture

@Jaza - well, the KISS principle suggests that assigning arbitrary menus to Primary/Secondary is not really needed.

What's the burning, common use case?

Jaza’s picture

@pwolanin: see http://drupal.org/node/170322 for a use case (not 'burning', I admit, but it's there).

pwolanin’s picture

Status: Needs work » Postponed (maintainer needs more info)

@Jaza - that issue seems somewhat orthogonal, unless you are saying that you want a book to be your primary links?

pwolanin’s picture

Status: Postponed (maintainer needs more info) » Closed (fixed)

in the absence of a compelling use case from Jaza, I'm satisfied with what we have already.

wwmv’s picture

Version: 6.x-dev » 6.0-beta1

Hi.
if you set "source for secondary links = primary links" and activate the "secondary links box" you get an empty box because links from the menu called "secondary-links" are shown.

I changed the following in function menu_block($op, $delta) to get it working:

  else if ($op == 'view') {
    $data['subject'] = check_plain($menus[$delta]);
    if ($delta == 'secondary-links' && variable_get('menu_secondary_links_source', 'secondary-links') == 'primary-links') {
      $tree = menu_tree_page_data('primary-links');
      foreach ($tree as $link_data) {
        if ($link_data['link']['in_active_trail']) {
          $data['content'] = menu_tree_output($link_data['below']);
          break;
        }
      }
    }
    else {
      $data['content'] = menu_tree($delta);
    }
  return $data;
}

hope this is the right place. i'm not sure if it is the right way to do....

Boris Mann’s picture

Status: Closed (fixed) » Active

Use case for arbitrary menus: I create a whole new menu and then want to set it to be primary, instead of manually fiddling with primary in real time. But I'll leave that horse for now and just note that this is re-opened with the bug fix code listed.

pwolanin’s picture

Version: 6.0-beta1 » 6.x-dev

@markus-v-drupal: this should not be part of this issue. A new issue is here: http://drupal.org/node/180786 please post follow-ups there.

@Boris: I still don't think assigning arbitrary menus is a good idea. I'd be much more supportive of adding utility function/admin page to allow two menus to be swapped. Or that could be a contrib module.

catch’s picture

Version: 6.x-dev » 7.x-dev
Category: bug » feature
David_Rothstein’s picture

Version: 7.x-dev » 6.x-dev
Status: Active » Fixed

This was fixed in 6.x as a result of http://drupal.org/node/216813

Anonymous’s picture

Status: Fixed » Closed (fixed)

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