Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#13669 closed defect (bug) (fixed)

Nav menu parameters not working

Reported by: vteixeira's profile 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 14 years ago.

Download all attachments as: .zip

Change History (11)

#1 @ocean90
14 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
14 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
14 years ago

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

#4 @nacin
14 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
14 years ago

#5 @nacin
14 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
14 years ago

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

#7 @vteixeira
14 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
14 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
14 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
14 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.