WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#13669 closed defect (bug) (fixed)

Nav menu parameters not working

Reported by: vteixeira Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch
Focuses: Cc:

Description

I have this:

wp_nav_menu( array( 'menu' => 'top', 'sort_column' => 'menu_order', 'container_id' => 'nav' ) );

The 'container_id' is ignored.

Then I looked at twentyten just to see if it was my fault and found out that it has 'container_class' => 'menu-header' specified and it's also ignored.

Please excuse if it's a duplicate. I made a quick search and could not find it on another ticket.

Using wp 3.0RC1

Attachments (1)

13669.diff (3.6 KB) - added by nacin 7 years ago.

Download all attachments as: .zip

Change History (11)

#1 @ocean90
7 years ago

  • Milestone 3.0 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Hi, there are three parameters:

  • container: default is div, if it's empty container_class will not work
  • container_class: class for the container parameter
  • container_id: Not for the container parameter. It's for the list, example: <ul id="nav">

The HTML Output of:
wp_nav_menu( array( 'menu' => 'top', 'sort_column' => 'menu_order', 'container_id' => 'nav', 'container_class' => 'menu-header' ) )
is:

<div class="menu-header">
  <ul class="menu" id="nav">
    <li class="menu-item menu-item-type-custom" id="menu-item-10">
      <a href="http://google.de">gg</a>
    </li>
  </ul>
</div>

Maybe you can update to the latest nightly and then check it again. If your problem still exists you can re-open.

#2 @vteixeira
7 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

So, there's something very wrong here.

If the container_class applies to the container (<div>), why the container_id applies to the menu (<ul>)?

I think the container_class and menu_class parameters are working as expected but the container_id is totally wrong.

On the Codex it says:

$container_class

(string) (optional) The parent element class

Default: blank

$container_id

(string) (optional) The parent element id parameter

Default: blank

So both applies to the same element - <div> (the container)

But it's not what we get on the output. So I think it must be fixed by applying the container_class parameter to the correct element.

I'll reopen.

#3 @vteixeira
7 years ago

Sorry, it's the container_id parameter that should be applied to the correct element.

#4 @nacin
7 years ago

  • Keywords needs-patch added
  • Milestone set to 3.0

I agree, though honestly, I hate this container code, I think it is an unnecessary complexity.

Let's re-assign container_id to the container, and add menu_id that adds to the ul.

@nacin
7 years ago

#5 @nacin
7 years ago

  • Keywords has-patch added; needs-patch removed

I think I'd rather kill all container code and just let the theme choose to include a container around their menu call. The only complexity that introduces is if the user declines to assign a menu to that location, which seems like a job for has_nav_menu().

IMO, unless it's a <nav> element purely for semantics, there's almost never a reason for an extra container div. I consider it cruft.

Nonetheless, has-patch.

#6 @nacin
7 years ago

The $container_id_slugs static var is unused and can be removed.

#7 @vteixeira
7 years ago

I agree, the output of the wp_nav_menu should be just the list:

<ul><li></li></ul>

If I want a container I should write it on my theme.

It should not force a container.

#8 @ocean90
7 years ago

Sorry, after re-reading my closing was stupid.
+1 for removing, wp_list_pages() and wp_page_menu() doesn't have a container parameter too.

I think we should also remove 'menu_class' and 'menu_id' to be consistent with wp_list_pages() and wp_page_menu().

#9 @ryan
7 years ago

It's too late to kill the container code. We're RC1 and many themes have already ported over. The container arg can be used to turn off outputting a container, which seems sufficient.

#10 @nacin
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [15113]) Add menu_id to wp_nav_menu() and move container_id to the container. Set the container arg to false to not use a container. fixes #13669

Note: See TracTickets for help on using tickets.