WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 13 months ago

#28620 reviewing defect (bug)

When $depth argument is used in wp_nav_menu last level menu items still have .menu-item-has-children class, even though they are not shown

Reported by: slobodanmanic Owned by: chriscct7
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: Menus Keywords: good-first-bug needs-testing has-patch
Focuses: ui Cc:

Description

Example:

  • Set wp_nav_menu $depth to 3
  • A theme has some CSS to indicate submenus (e.g. Twenty Fourteen)
  • If last level (as allowed by $depth argument) items still have children, they will not be shown, but .menu-item-has-children class is still there

I don't think the class itself is the problem, because the menu item does have children, they're just not being displayed. It might be good to add a new class that would help theme developers disable submenu indicator for menu items in which $depth prevents sub-items to be shown.

Maybe something like this (in nav-menu-template.php):

function start_lvl( &$output, $depth = 0, $args = array() ) {
	$indent = str_repeat("\t", $depth);
	$sub_menu_class = ( $depth + 2 == $args->depth ? 'submenu menu-last-level-parent' : 'submenu' );
	$output .= "\n$indent<ul class=\"$sub_menu_class\">\n";
}

This would allow .menu-last-level-parent li selector to disable dropdown indicator, since the indicator is not needed when sub-items are not shown.

$depth +2 feels hacky to me, but this is by far the easiest way to add the class. Ideally, class would be added to last level <li> elements, but I think that would require updates to Walker class.

Attachments (3)

28620.diff (943 bytes) - added by iCaspar 2 years ago.
28620.2.diff (680 bytes) - added by mdgl 15 months ago.
28620-alt.diff (1.8 KB) - added by mdgl 13 months ago.
Alternative (preferable?) patch

Download all attachments as: .zip

Change History (14)

#1 @slobodanmanic
3 years ago

  • Version changed from trunk to 3.7

#2 @slobodanmanic
3 years ago

  • Summary changed from When $depth argument is used in wp_nav_menu last level menu items still have .menu-item-has-children class to When $depth argument is used in wp_nav_menu last level menu items still have .menu-item-has-children class, even though they are not shown

#3 @SergeyBiryukov
3 years ago

Introduced in [25602].

#4 @slobodanmanic
3 years ago

Worth noting that this can be achieved by using .menu li li li li type selectors, but last level class would be cleaner.

#5 @kucrut
3 years ago

Here's one approach:

public function start_el( &$output, $item, $depth = 0, $args = array(), $id = 0 ) {
	$indent = ( $depth ) ? str_repeat( "\t", $depth ) : '';

	$classes = empty( $item->classes ) ? array() : (array) $item->classes;
	$classes[] = 'menu-item-' . $item->ID;

	if ( ( -1 === $args->depth ) || ( $args->depth > 0 && ( ( $depth + 1 ) === $args->depth ) ) ) {
		$_classes = array_fill_keys( $classes, '' );
		unset( $_classes['menu-item-has-children'] );
		$classes = array_keys( $_classes );
	}

	// ...

If only nav_menu_css_class is passing the current $depth then all this could be done with a simple filter callback.

I'll be happy to format this as a patch if it's considered the right approach.

#6 @wonderboymusic
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#7 @chriscct7
2 years ago

  • Keywords good-first-bug needs-testing added

Good First Bug: Attempt to verify example is still valid on latest stable

@iCaspar
2 years ago

#8 @iCaspar
2 years ago

  • Keywords has-patch added; needs-patch removed

Here's the patch based on @kucrut's suggestion above. It seems that removing the .menu-item-has-children route is the better one for two reasons:

  1. Seems more backwards-compatible, as themes won't have to update their menu CSS to take into account the addition of a new class, and
  2. Even though the menu item does have children in terms of the menu walker, because the depth has been limited, they won't ever see the light of day on the front end. At that point there's no use in having a CSS class that says an item on the page has children when it doesn't.

#wcus

Last edited 2 years ago by iCaspar (previous) (diff)

#9 @chriscct7
2 years ago

  • Owner set to chriscct7
  • Status changed from new to reviewing

@mdgl
15 months ago

#10 @mdgl
15 months ago

Patch refreshed. This should perhaps even be categorised as a "bug" rather than an "enhancement". Note that a similar issue exists within class Walker_Page and likely other "walker" classes. These remain unpatched.

@mdgl
13 months ago

Alternative (preferable?) patch

#11 @mdgl
13 months ago

  • Type changed from enhancement to defect (bug)

Alternative patch added, which avoids adding the incorrect class names at source. This might be considered preferable. It will use a little more time/space but this shouldn't be a problem as most menus will be (relatively) short and shallow. Also re-categorised ticket as a "bug".

Note: See TracTickets for help on using tickets.