WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#13378 closed defect (bug) (fixed)

Associate nav menus with particular menu locations in a theme

Reported by: ryan Owned by: nacin
Milestone: 3.0 Priority: normal
Severity: blocker Version:
Component: Menus Keywords: ux-feedback
Focuses: Cc:

Description (last modified by ryan)

There is currently no way to associate a particular menu with a given call to wp_nav_menu() in a theme. Themes cannot call wp_nav_menu() multiple times and have sane assignment of menus without hard coding particular menu names/ids/slugs (which is not portable across themes and languages).

This can be solved by allowing themes to register how many menus they use and give those menus a handle. A dropdown in the menus admin would show these handles and allow associating a menu with a given handle. Upon switching themes this association would be cleared, although a set of standard slot names could be used and preserved when switching themes (Main, Secondary, ...). Registering "slots" decouples assignment from menu naming, allowing users to name their menus as they see fit in any language instead of being tied to hard-coded strings coming from the theme.

Attachments (12)

13378.diff (1.7 KB) - added by ryan 4 years ago.
Beginning of register_nav_menus()
13378.2.diff (3.7 KB) - added by ryan 4 years ago.
13378.3.diff (3.7 KB) - added by ryan 4 years ago.
nav menu locations.png (48.4 KB) - added by TECannon 4 years ago.
nav-menu-locations mockup
13378.truncate.long.names.1.patch (793 bytes) - added by koopersmith 4 years ago.
14720.patch (2.9 KB) - added by jorbin 4 years ago.
inline docs for some functions added/changed
13378.nonce.diff (2.0 KB) - added by koopersmith 4 years ago.
13378.4.diff (688 bytes) - added by ryan 4 years ago.
Explanatory text for Theme Locations box.
13378.add.menu.save.diff (3.5 KB) - added by koopersmith 4 years ago.
13378.location.wording.diff (848 bytes) - added by johnonolan 4 years ago.
Revised wording for selecting a location or locations based on my comment above.
13378.twentyten.menulocation.label.diff (515 bytes) - added by johnonolan 4 years ago.
Revised menu location label for TwentyTen.
fix.underlines.diff (829 bytes) - added by koopersmith 4 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 ryan4 years ago

  • Description modified (diff)

ryan4 years ago

Beginning of register_nav_menus()

comment:2 ryan4 years ago

Themes do this in functions.php:

register_nav_menus( array('main' => __('Main Menu'), 'footer' => __('Footer Menu') ) );

And this in templates:

wp_nav_menu( array('slot' => 'main') );
wp_nav_menu( array('slot' => 'footer') );

In the backend, an array is set in options that is keyed by slot with the values being menu ids.

update_option( 'nav_menu_slots', array( 'main' => 1234, 'secondary' => 5678 ) );

ryan4 years ago

comment:3 ryan4 years ago

During dev meeting it was decided to store the slots per theme.

ryan4 years ago

comment:4 wpmuguru4 years ago

(In [14611]) add menu slots/theme_menus, props ryan, see #13378

comment:5 ryan4 years ago

Todo:

  • Actually save the slot mapping
  • Allow a slot mapping of none in the dropdown
  • Make sure two (or more) menus don't map to the same slot
  • Update stale mappings when switching back to a theme
  • Assign default slot mappings when switching to a theme for the first time
  • Find a better term than slot
  • UX/UI review, cause this is confusing and needs love

TECannon4 years ago

nav-menu-locations mockup

comment:7 nacin4 years ago

  • Owner set to nacin
  • Status changed from new to accepted

In progress.

comment:8 nacin4 years ago

(In [14620]) First pass on 'Theme Locations' for navigation menus. Themes need to specify a location when calling wp_nav_menu and register locations in functions.php. Users then map menus to locations in the nav menu admin. Subject to review. see #13378.

comment:9 nacin4 years ago

I wanted to get the basic structure committed first so others can see what we are confusing each other about.

This still needs some love.

UI - I imagine we will want to visually separate this meta box from the others. Maybe even lock it to the top (and allow it to be collapsible, but kick it out of screen options), and shade it like a widget box (see mock from Tracy above).

JS - Currently it works non-JS. So now we just need to add progressive enhancements. I am thinking about this in two stages. One, the "Save" button needs to save them via AJAX. In fact, the button (when JS) should probably be disabled unless the meta box is dirty, simply because it's an asynchronous setting any way you look at it.

Two, the "Save Menu" button needs to copy over these fields and post them with the form, otherwise they will not be saved if the "Save" button was not pressed. That ensures everything being seamless.

UX - Finally, we need some UX feedback here. But I think we should bite our tongues a little until the UI and JS stuff gets farther along.

Feel free to run with this farther without me.

comment:10 nacin4 years ago

(In [14621]) Only show Theme Locations meta box if menus exist and if the theme has registered menus. see #13378.

comment:11 nacin4 years ago

(In [14623]) If theme has registered menus, set the first menu created to the first registered location by default. see #13378.

comment:12 demetris4 years ago

  • Cc dkikizas@… added

comment:13 ryan4 years ago

  • Summary changed from Associate nav menus with particular menu "slots" in a theme to Associate nav menus with particular menu locations in a theme

comment:14 WraithKenny4 years ago

  • Cc Ken@… added

comment:15 nacin4 years ago

(In [14671]) Truncate long menu names in the menu locations box. props koopersmith, see #13378.

comment:16 ryan4 years ago

  • Keywords ux-feedback added

comment:17 follow-up: nacin4 years ago

(In [14715]) Save menu locations meta box via ajax. see #13378.

jorbin4 years ago

inline docs for some functions added/changed

comment:18 jorbin4 years ago

  • Cc aaron@… added

Some of the functions added / modified lack proper documentation. The above patch fixes that.

comment:19 nacin4 years ago

(In [14721]) Inline docs for menu location functions. props jorbin, see #13378.

comment:20 in reply to: ↑ 17 TobiasBg4 years ago

Replying to nacin:

(In [14715]) Save menu locations meta box via ajax. see #13378.

No nonce check or check_ajax_referer()? Is it not necessary here?

comment:21 ryan4 years ago

There should be a nonce. We need a nonce audit in admin-ajax. I see another place that needs it.

comment:22 nacin4 years ago

Yeah, we need one there.

koopersmith4 years ago

comment:24 ocean904 years ago

The saving doesn't work. We send via AJAX

menu-locations	menu-locations%5Bprimary%5D=

and get_nav_menu_locations() returns a string

string(30) "menu-locations%5Bprimary%5D=81"


But it should be an array I think: $menu_locations[$location]

ryan4 years ago

Explanatory text for Theme Locations box.

comment:26 jorbin4 years ago

The commit message didn't go through. [14772]

comment:27 johnonolan4 years ago

UX: This looks good so far

comment:29 ryan4 years ago

The patch on that thread would have the affect of using the callback when a theme location is not assigned to a menu even if menus exist and there is only one theme location. Since we automatically assign a location to a menu when the menu is created, the location should only be unassigned due to explicit action by the user. This seems okay and provides a means for the user to explicitly ask for the theme's fallback to be run without having to delete menus.

comment:30 ryan4 years ago

(In [14793]) Don't look for a fallback menu when a specific location is requested. Props tinkerpriest. see #13378

comment:31 ryan4 years ago

What's left?

comment:32 ryan4 years ago

I think this is looking pretty good. If you guys are satisfied, resolve as fixed.

comment:33 johnonolan4 years ago

Couple of things:

UX:
The wording is currently "Your theme supports [1] [menu/menus]. Assign a menu to your theme." - Would it be better as "Your theme supports [1] [menu/menus]. Select where you would like [it/them] to appear." ?

The label above the dropdown is "Primary Menu" - I'm thinking that this label should describe the location rather than the menu. Suggested alternative: "Primary Navigation" or "Main Navigation".

UI:
Could we make the dropdown the same height as the text fields in the custom links metabox? I think it's a couple of pixels out at the moment

comment:34 nacin4 years ago

Aside from UI/UX, there is one bit of functionality remaining. When "Save Menu" is pressed, it also needs to save the theme locations. Obviously, this would only be if JS. Anyone is feel free to pick up this.

johnonolan4 years ago

Revised wording for selecting a location or locations based on my comment above.

johnonolan4 years ago

Revised menu location label for TwentyTen.

comment:35 nacin4 years ago

(In [14831]) Menu tweaks. Save menu locations when saving the menu. Also, centralize theme support checks and add them to menu.php. Improve some branching. Remove some old JS vars, add a missing semicolon, etc. props koopersmith. see #13378.

comment:36 nacin4 years ago

(In [14832]) Some menu string improvements. props JohnONolan. see #13378.

koopersmith4 years ago

comment:37 nacin4 years ago

(In [14834]) Styling for menu locations howto text. see #13378.

comment:38 automattor4 years ago

(In [14846]) Better text for locations box. Props filosofo. fixes #13520 see #13378

comment:39 ryan4 years ago

(In [14848]) Fix styling of underlines for menus admin. Props koopersmith. see #13378

comment:40 nacin4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.