Finer control over the Parent Menu select box

Note that if we combine this with #273137: Split Navigation to User and Administration menu, we could then also set things up out of the box so that the administrative menu items don't show up as available parents when creating story and page nodes. Seeing all those admin items as options confused the heck out of people in the usability testing, so that would be a big win. Has anyone EVER added a story or page to the admin area? I sure haven't, but you have to stumble over all those options every time.

subscribing.

subscribing

Patch re-roll and screenshots showing the change for the patch averse.

changing status to needs review

Tagging.

We're about to try this at Usability testing after having major menu module issues.

Here's a slight update which removes the two default menu items from the default profile - because people were choosing these as menu parents thinking it'd create a new menu item (similar to how book module does in it's select list).

Also I think we should make the UI for selecting if a menu appears in the parent select a checkbox when editing a menu rather than in the settings tab.

fixing tag.

I think we should move to the 2x select like the book module has - I hate the single chooser of the menu system.

Maybe revive #191360: Scalable menu selector ?

subscribing.

I'd like to see this enhancement include the appropriate hooks to allow modification of Parent select options based on user permissions, as well as node type. I'm looking for the ability to restrict where in the site hierarchy my users are allowed to put a node, based on who they are and what kind of content they are uploading.

For instance, here at the California Science Center, I want an exhibit curator to have complete freedom when it comes to creating content that deals with their exhibit gallery. However, they should only be able to put this content in the website section dealing with that exhibit gallery. Our Air & Space curator should never be given the option of sticking his page about the Apollo space capsule in the menu for our IMAX Theater. Every option in the Parent selector should be grayed-out except for those under the Air & Space Gallery section.

If you give users the option of making a mistake, they will make it. Maybe not all the time, but often enough.

@jstoller - would the idea of nested menus suit this? I don't know if it has been suggested before, or if it would fit with this issue, but could be a way of doing it.

The way I see it working is to create x menus which refer to different parts of the site, and then a special menu which has the other menus as it's children.

eg you have an Air & Space menu, an IMAX Theatre menu etc, which could then be grouped under one big Navigation menu which the administrator can build. Then you set permissions per menu, which delegates control but allows a constant, single site hierarcy.

Nested menus might help in some cases, but I fear they would quickly become unmanageable. I can easily see a website degenerating in to a tangled mess of nested menus within nested menus, making it impossible to find anything. From a usability standpoint, it seems dangerous to break up the visual of a unified menu hierarchy.

I would prefer adding the ability to assign permissions to branches of the hierarchy directly. I see this happening in two complimentary ways. First, when I create a content type I should be able to restrict where in the menu hierarchy nodes of that type can be placed. Second, when I edit a menu item I should be able to restrict that branch of the menu to specific user roles. When creating a node of a given content type, the menu parent selector would only show menus that work with that content type and which my user roles allow me to access. Within a given menu, only the subset of the hierarchy which I (and the content type) can access should be selectable. The rest of the hierarchy must still be shown (so I know where I am in the site), but should be grayed-out.

This same idea should be carried over to the menu settings page as well. I should only be able to rearrange the subset of a menu's hierarchy that I have permission to manage. The rest of the menu items should be grayed-out and unmovable.

I expect some of these UI refinements could be done in contrib initially, as long as the underlying menu system is built to support the functionality.

I think this all a bit of a derailment. The purpose of this patch is to simplify the UI, not make it overly complicated and allow you to allow you to use specific branches of the menu depending on user role, which is all doable in contrib anyway.

I think we should keep things simple here and offer one option per menu, which is whether or not it shows up as a parentable item.

didn't completely review #9, but this looks wrong:

+  $in_admin_page = arg(0) == 'admin';

I'm sorry I pulled this off topic with the user permissions angle, but I'm happy to hear it is possible to implement in contrib.

While I agree this patch is a step in the right direction, I do think it would greatly improve usability if the menus could be associated with content types, rather than just enabled or disabled for all parent selectors. However, I'm happy to request that feature in a separate issue.

Hi,

I use your patch it is nicely working but One problem related multilingual site is when I have created the page in dutch under primary-links menu to sub menu item. When i go to edit the page the sub menu do not come to be selected by default.

I am not pointing to your patch solution because by default in drupal case i got the same problem. So if you know the solution for that than please let me know.

As you can see in screen shot.

I have use i18n and language icons module for translations.

Thank you.

please don't change the title like that

Related issue: http://drupal.org/node/417106
Remove Management Menu from Parent Selection on Node Form

pwolanin, what's a better way to check if we are in the admin section and, thus, need to override the "hide this menu" functionality?

$in_admin_page = arg(0) == 'admin'; works. But maybe we need to add a flag to toggle that behavior? :-\ Just thinking out loud.

Talked with kika about this and we came up with the attached mockup.

The design is definitely not considered ‘finished’ but we think the content type form is a very logical place for these settings. What are your thoughts?

Updated mockup with some textual changes and smart defaults.

We are progressing with the design. After discussion with Dries, Bohjan and Roy we keep the proposed design, just adding more clarity:

- By default, these are the default menu-is-enabled-in-node-add-menu settings per content type:

Main menu: enabled
Management: disabled
Navigation: disabled
Secondary menu: enabled
User menu: disabled

- When user adds new menu, the default menu-is-enabled-in-node-add-menu setting for each content type is enabled

Interaction:

- If user enables / disables menus to display, the default menu item dropdown contents will be updated dynamically -- adding and removing dropdown options
- When user have selected and item from the particular menu and then disables that menu, the default selection falls to the first element remaining in the dropdown
- When user have disabled all menus to display, all elements will be removed from the dropdown AND in node authoring page the "Menu settings" vertical tab is not displayed

Forgot to include the user menu — sorry.

Subscribing.

kkaefer, how is it going with the patch?

kika - I completely agree with all points in #26. A few questions:

- Do these limitations effect all users, including admin? Should permissions be taken into account?
- By default, should the article content type have any menus available? IMO, removing the menu fieldset/vertical tab could help ease some of the confusion regarding the difference between a page and article.
- Should there be an additional option to require adding a page to a menu item? Perhaps that is out of scope for this issue.
- (More kitten killing) Moving menu selection into vertical tabs doesn't always make sense if a content type requires it. For example, when creating a page the menu fieldset should be front and center. For content types that that normally do not require a menu, I think hiding the option in vertical tabs makes sense. Perhaps this could be tied to whether adding a menu is required. Alternately, re-arranging the menu form may be best addressed as part of the CCK UI or Fields UI in Core.

Don't think anyone is working on this. Marking this crtical, since it causes a lot of initial usability problems.

- All users should be affected by this
- I think articles should have menus available.
- Out of scope
- Out of scope as well, this option should probally be tab 1 if required.

critical indeed.

eigentor asked me to provide a patch for the last shown solution so here it is.

some screenshots.
#1 is showing the content type settings page with the new "menu settings" fieldset
#2 is showing the menu settings on node creation with pre-selected parent menu item and reduced option list

This looks great, minor whitespace issue with

+    foreach ($type_menus as $menu) {
+    	$available_menus[$menu] = $menu;
+    }
+    
+  	$item = array('mlid' => 0);
+  }

I'd like to add some variable_set() in the default install profile - to disable the management menu (probably others as well) - could maybe be a separate patch though.

Hm, don't know where these tabs came from. The editor is set up to use spaces only.
Anyway, here's a new patch with corrected whitespace.

Spotted a couple more things:

     return array();
   }
+  $available_menus = menu_get_menus();
+  // If the item is a node type, get all available menus for this type and prepare a dummy menu item.
+  if (!is_array($item) && isset($item)) {
+    $type_menus = variable_get('menu_options_' . $item, array('main-menu' => 'main-menu'));
+    $available_menus = array();
+    foreach ($type_menus as $menu) {
+      $available_menus[$menu] = $menu;
+    }
+
+    $item = array('mlid' => 0);
+  }

$menus is a parameter to the function, and then $available_menus is almost immediately reset when it's set to an empty array - shouldn't we be loaded it later only if needed? Only reading the patch out of context but it looks like we probably don't need to fetch the menus again at all.

t('Choose the menu item to be default for the %menu_settings in the content autherinig form.', array('%menu_settings' => t('menu settings'))),

authering -> authoring. No need to use a placeholder for %menu_settings

Ok, I've changed the code to take the given menu items and don't fetch them again.

No need to use a placeholder for %menu_settings

Hm, I acted upon the last mockup in #27.
If you say "no emphasize" I'll go with it.

You can happily put <em> tags inside t() strings if you want (same with strong etc.), no need to use placeholders for that.

Ok, I've done it because "menu settings" is the title of the fieldset on node creation and I thought it will be more consistent to translate it only once :-).
But I'm ok with no placeholders.

OK really sorry this took three passes. The changes to content_types.inc should be in hook_form_node_type_form_alter() - menu module is optional whereas node module isn't, and it saves the module_exists() check. See http://api.drupal.org/api/function/comment_form_node_type_form_alter/7

Other than that looks RTBC to me.

Ok, now with hook_form_FORM_ID_alter() (don't know why I didnn't thought about it ...).

Looks great now.

Spend quite some time going over this, looking at the possible alternatives. But with the restraints, I feel this solution is good. I hope the sensible defaults of excluding management menu or whatever the toolbar uses is done correctly.

Looks RTBC from the UX perspective from me.

Just skimming through the code:

- should we get rid of "Default menu for content" in admin/build/menu/settings ?

- what about upgrade path with that 'menu_default_node_menu' value populating fo all content types 'menu_parent_' . $type->type

- when I unselect all menus in content type form and submit, and try to add that content type node, I will get

"Notice: Undefined variable: options in menu_parent_options() (line 246 of /Users/kristjanjansen/Sites/drupal7/modules/menu/menu.module)."

This is wrong, instead the whole menu vertical tab should go away

- should not "Secondary menu" be enabled by default too?

- What becomes the default value when user selects a menu from the list what is not enabled?

(proposed interaction in #26: If user enables / disables menus to display, the default menu item dropdown contents will be updated dynamically -- adding and removing dropdown options; When user have selected and item from the particular menu and then disables that menu, the default selection falls to the first element remaining in the dropdown)

BTW, kkaefer was working on the same patch, can he provide his work so far?

So, obviously needs work

- should we get rid of "Default menu for content" in admin/build/menu/settings ?
Yes, but follow up?

- when I unselect all menus in content type form and submit, and try to add that content type node, I will get
bug

- should not "Secondary menu" be enabled by default too?
Not sure, because this is the default for all other themes as well.

- What becomes the default value when user selects a menu from the list what is not enabled?
The behavior you proposed, seems like the one we pursued?

Next try.
This time it hides the vertical tab if there are no menus selected.
If you select a disabled menu item as parent it is pre-selected on node creation.

While I like this patch, I think it does a poor job explaining what this setting is for. If I were a user new to Drupal, this fieldset would confuse me. There is no context.

Related to that: I don't understand "Menus which should appear in the add menu item section." What is the 'add menu item section'? On the node edit form it is called 'menu settings'?

-

I think Dries is right.

"Menus which should appear in the add menu item section."

Should be something like "The menu's available to place links in for this content type.".

Also, why does Default parent item not update anymore on the given selection above?

We had a discussion at the UX-Sprint about this all : http://vimeo.com/groups/drupalux/videos/5676880

Ok, the latter description sounds much better.

why does Default parent item not update anymore on the given selection above?
Uhm, I never saw this before (or can't remember). Do you mean updating the default parent item based on the selected menus? Dynamically?

Yes, I was under the impression that - what I select influences the box below.

Hm, well. This has to be done dynamically.
I'll have to look into some code to figure out how to do this. Stay tuned.

Here's a work in progress. There are still some UI glitches but it should work overall.

Heres my try with dynamically loading the available default menu items.
Its a different approach to kkaefer's patch because it only show menu items from the currently selected menus and did not just disable them.
I know the code could be better (and should be cleaned up) but its a hint how it could look like.
If you deselect all menus, save the content type and then try to select a menu and save the content type again there is an error about an illegal option. Don't know how this can be avoided.

Uh, I forgot to include the new menu.admin.js. Now it should work (apart from this annoying "illegal choice" error).

Oke, looking good - probably needs some cleanup as the behavior is somewhat funky now sometimes. We do need to work on the indicator that it has changed.

Lets get some reviews up.

Well, I did some more testing and found why its not working properly.
If you add some items to the select element ("Default parent item") the form element itself does not know anything about these new items.
Therefore the validation will fail if you select one of the new fields (because its key isn't in the options array of the select-element).

Question: how do we update $form['menu']['menu_parent']['#options'] with the values returned by menu_parent_options_js()? Is this possible at all?
If not, you can't update the available options in "Default parent items" after (de-)selecting one of the "Available menus" checkboxes via AJAX. You'll have to select the available menus first, save and then select the default parent item.

Thoughts?

Update the patch: now there are no errors after selecting an item from the "Default parent item" dropdown that was added right before.
See #351249-34: Finer control over the Parent Menu select box for Screenshots (UI did not change).

Subscribe.

Bot? Are you sure?

Reviewed, looks good, changed one line where atributes class should be an array.

Only one confusing thing for me was:
Main menu is the default menu selected. While adding content (node/add) I see a link called "Add a main menu link". Almost it looked like an action to add a new menu item that needs to be selected.

That is confusing but can be fixed elsewhere.

@sign: you forgot to include menu.admin.js

oops, sorry for that :( will make sure to always run patches against clean cvs checkout

reviewed, works fine for me

I applied it, and it behaves as I expected it now.

I don't understand why the menu settings aren't part of the Vertical Tabs? That is confusing me.

Vertical tabs got submitted in between the progress of this patch?

@Dries: ah, ok. I simply forgot this :-)

New screenshots.

Oh and I forgot the new patch (menu.module has changed so it needed a re-roll).

tested, works as expected, screenshots attached :)

+            $('<option ' + (index == selected ? ' selected="selected"' : '') + '></option').val(index).text(value)

There’s a > missing here.

Added the missing character.

Back to RTBC, this has been reviewed many times now - lets get this sucker in.

Rerolled the patch.

Time to put this back to RTBC.

This patch needs a bit more work.

+++ modules/menu/menu.admin.js	10 Oct 2009 10:55:38 -0000
@@ -0,0 +1,48 @@
+    var vals = [];

We don't abbreviate variables to 'vals'. Write 'values' or 'items' or something else.

+++ modules/menu/menu.admin.js	10 Oct 2009 10:55:38 -0000
@@ -0,0 +1,48 @@
+      // Get the name all checked menus.

This isn't proper English.

+++ modules/menu/menu.module	10 Oct 2009 10:55:40 -0000
@@ -309,6 +315,44 @@ function menu_parent_options($menus, $it
+    // If $item is a node type, get all available menus for this type and prepare a dummy menu item.

Please describe why we prepare a dummy menu item.

+++ modules/menu/menu.module	10 Oct 2009 10:55:40 -0000
@@ -309,6 +315,44 @@ function menu_parent_options($menus, $it
+  if (isset($_POST['menus']) && count($_POST['menus'])) {
+   foreach ($_POST['menus'] as $menu) {
+     $available_menus[$menu] = $menu;
+   }
+  }

Indentation is not according to the coding standards.

+++ modules/menu/menu.module	10 Oct 2009 10:55:40 -0000
@@ -554,6 +609,52 @@ function menu_form_alter(&$form, $form_s
+  // Get all menu items here. If we would provide only available items here
+  // FormsAPI would not allow us to add other items dynamically and select
+  // one of the new items before saving the form. It would show an
+  // 'illegal option' error and cancel the form submission.

This needs some massaging. It reads like broken English. Also, "FormsAPI" should be "Form API".

+++ modules/menu/menu.module	10 Oct 2009 10:55:40 -0000
@@ -554,6 +609,52 @@ function menu_form_alter(&$form, $form_s
+  // Initially update the list of available menu parent items so we only have
+  // the items from the selected menus.

I don't understand this code comment.

Hope this one is better.

Forget my comment. Installed the patch, fixes the issue I lamented about.

OK, this looks good enough. Committed to CVS HEAD. We can refine in follow-ups as necessary.

#86 dealt with all of Dries' points. I found a couple of comments which had a summary more than one line, changed Javascript to JavaScript - rather than mark CNW I re-rolled the patches with these changes and am marking RTBC. Please don't credit on commit.

Heh.

None of those were major, we can do those in the 'fix code comments' issues in about a month.

This was not documented in the update guide, now added http://drupal.org/update/modules/6/7#menu_default_node_menu

This change is missing an upgrade path (although noted by @kika in July 2009 that it would need one). Critical issue opened at #761648: Menu D6->D7 upgrade doesn't maintain node-menu configuration (f.k.a.: Menu items disappear after upgrade or manual menu entry).