WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 9 days ago

#36163 closed enhancement (fixed)

Make Walker_Nav_Menu::start_lvl more flexible by introducing filters for class name(s) at least

Reported by: ruslik Owned by: peterwilsoncc
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Menus Keywords: needs-unit-tests good-first-bug has-patch
Focuses: Cc:

Description

It seems that most of the posibilities to "tune" menus output through filters in Walker_Nav_Menu::start_el, have limited use cases without the ability to alter Walker_Nav_Menu::start_lvl output?

Attachments (4)

36163.diff (1.1 KB) - added by csloisel 3 months ago.
Adding submenu css class filter in start_lvl()
add_filter_nav_submenu_start_lvl.patch (2.6 KB) - added by darthaud 7 weeks ago.
Add css filter and id filter to walker nav menu Class
36163.2.diff (999 bytes) - added by raisonon 4 weeks ago.
36163.3.diff (1.2 KB) - added by Kopepasah 2 weeks ago.

Download all attachments as: .zip

Change History (24)

#1 @ruslik
15 months ago

  • Component changed from General to Menus

#2 @chriscct7
14 months ago

  • Version trunk deleted

#3 @welcher
4 months ago

  • Keywords reporter-feedback added

Thanks for the report!

Can you be a bit more specific as to the request, perhaps adding a patch or some pseudo code?

#4 follow-up: @ruslik
4 months ago

Sure.

So currently if user/theme developer want to customize Navigation output, she/he has two options (as I'm aware of):

  1. Extend Walker_Nav_Menu class and overwrite its methods (which in some cases maybe an overkill).
  2. Hook into the following filters which unfortunately only defined for nav item markup output.

So we have a bunch of hooks that allow us to customize item markup, but zero for level markup. I think it would be helpful to introduce similar filter hooks for start_lvl method, which responsible for ul markup.

Hope it clears things a little bit, and make sense. Thanks.

#5 in reply to: ↑ 4 @peterwilsoncc
4 months ago

  • Keywords needs-patch needs-unit-tests good-first-bug added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.8

Replying to ruslik:

Thanks for clarifying. I agree, adding a filter the sub-menu class would be a good thing. Naming the filter nav_menu_submenu_css_class and mirroring the arguments for the filter nav_menu_css_class.

Filtering the full output might be overkill, as end_lvl() would also need to be filtered at which point a custom walker isn't overkill.

#6 @ruslik
4 months ago

That's good.
What about nav_menu_submenu_id for id attribute as well, just in case anyone need additional flexibility?

EDIT: Although I see that it's not as trivial to implement it as nav_menu_submenu_css_class hook.

Last edited 4 months ago by ruslik (previous) (diff)

@csloisel
3 months ago

Adding submenu css class filter in start_lvl()

@darthaud
7 weeks ago

Add css filter and id filter to walker nav menu Class

#7 follow-up: @darthaud
7 weeks ago

  • Keywords has-patch added; needs-patch removed

#8 in reply to: ↑ 7 @ruslik
7 weeks ago

Replying to darthaud:

I think there also should be a way to generate unique IDs for sub-level ULs to make output valid HTML (with current patch I don't see a way to do it). Maybe introduce static class property with index or something like that and then add it to filter args.

#9 @peterwilsoncc
7 weeks ago

I'd like to limit any changes on this ticket to the class, rather than adding an ID too.

Adding an ID can be discussed in another ticket if needs be, as @ruslik mentions there's no way of ensuring the ID is unique in the current patch. A more complicated approach would need to be taken should it be decided to add them to core.

@raisonon
4 weeks ago

#10 @raisonon
4 weeks ago

In 36163.2.diff

  • Removed the ID completely from the code.
  • Introduced late escaping to the class name variable.

#11 @peterwilsoncc
4 weeks ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 40537:

Menus: Add filter to sub-menu class in nav menus.

Add new filter nav_menu_submenu_css_class to the Walker_Nav_Menu::start_el() method, allowing themers to modify the sub menu classes output by wp_nav_menu().

Props: csloisel, darthaud, raisonon.
Fixes: #36163.

#12 follow-up: @Kopepasah
2 weeks ago

@peterwilsoncc I think it might be better to make an empty array to not append and empty class= attribute to the <ul> element.

See: https://github.com/xwp/wordpress-develop/pull/228

@Kopepasah
2 weeks ago

#13 @Kopepasah
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#14 in reply to: ↑ 12 ; follow-up: @peterwilsoncc
2 weeks ago

Replying to Kopepasah:

@peterwilsoncc I think it might be better to make an empty array to not append and empty class= attribute to the <ul> element.

See: https://github.com/xwp/wordpress-develop/pull/228

I did consider this but ultimately decided escaping the attribute as $output was populated was the most appropriate course of action. The empty attribute is benign in the case a developer delete the default class rather than replacing.

#15 in reply to: ↑ 14 ; follow-up: @Kopepasah
2 weeks ago

Replying to peterwilsoncc:

Replying to Kopepasah:

@peterwilsoncc I think it might be better to make an empty array to not append and empty class= attribute to the <ul> element.

See: https://github.com/xwp/wordpress-develop/pull/228

I did consider this but ultimately decided escaping the attribute as $output was populated was the most appropriate course of action. The empty attribute is benign in the case a developer delete the default class rather than replacing.

Yeah, I do agree, but was following the way the menu item li was implemented. Seems awkward to implement the same solution two different ways in the same class.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


11 days ago

#17 follow-up: @pbiron
10 days ago

I like this and don't want to throw a wrench into this making it into the imminent 4.8 release, but it would be good to add it's equivalent to both the Walker_Page and Walker_Category classes as well (of course, adding those in 4.8.1 would be acceptable to me).

Related: #40359, #40666

#18 in reply to: ↑ 15 @peterwilsoncc
9 days ago

Replying to Kopepasah:

Yeah, I do agree, but was following the way the menu item li was implemented. Seems awkward to implement the same solution two different ways in the same class.

I've been sitting on the a few days pondering it, you've convinced me & I'll put it in this weekend.

#19 in reply to: ↑ 17 @peterwilsoncc
9 days ago

Replying to pbiron:

I like this and don't want to throw a wrench into this making it into the imminent 4.8 release, but it would be good to add it's equivalent to both the Walker_Page and Walker_Category classes as well (of course, adding those in 4.8.1 would be acceptable to me).

Related: #40359, #40666

If the don't already exist, could you please open separate tickets for the other walkers & send me a direct message in slack with the numbers?

#20 @peterwilsoncc
9 days ago

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

In 40665:

Menus: Prevent empty class attribute following [40537].

Prevents an empty class attribute, class="", from appearing in the HTML if a developer removes all classes using the nav_menu_submenu_css_class filter.

Props Kopepasah.
Fixes #36163.

Note: See TracTickets for help on using tickets.