WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 8 months ago

#41081 assigned enhancement

Improve Custom Menu widget, show notification if menu is empty or no menu selected

Reported by: alexvorn2 Owned by: mdifelice
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9
Component: Widgets Keywords: good-first-bug has-patch 2nd-opinion needs-testing needs-screenshots needs-refresh
Focuses: administration Cc:
PR Number:

Description

If you choose a menu for Custom Menu widget and we remove all items in that menu OR the menu is not selected -> nothing shows on the page.
Maybe we should add a text message that will inform the user that the menu is empty or is not selected?

Attachments (6)

class-wp-nav-menu-widget.diff (492 bytes) - added by mdifelice 2 years ago.
41081.patch (782 bytes) - added by umangvaghela123 2 years ago.
Fix #41081 issue.
41081.diff (552 bytes) - added by mdifelice 2 years ago.
Patch for hiding empty menus in Nav Menu Widget (differential from the root repository)
41081.2.diff (1.1 KB) - added by welcher 2 years ago.
Updating messaging to reflect the changes in this patch.
41081.3.diff (1020 bytes) - added by mdifelice 2 years ago.
Solved typo on new message.
41081.4.diff (9.4 KB) - added by mdifelice 2 years ago.
New patch with Customizer support.

Download all attachments as: .zip

Change History (31)

#1 @welcher
2 years ago

  • Focuses administration added
  • Keywords needs-patch good-first-bug added

@alexvorn2 thanks for reporting!

If you're meaning that the widget should output the message to visitors of the site, I don't think that is the best user experience. The author will still have to view the site to see that there is an issue with the widget so we're not much further ahead.

It's pretty clear in the widget admin when a menu is not selected but there is no indication whether the menu selected is empty. It might be a good idea to change the wp_get_nav_menus() call in the form() method of the widget to exclude empty menus with 'hide_empty' => true. This will exclude any empty menus from the list so that they cannot be selected.

#2 @mdifelice
2 years ago

Following @welcher recommendation I sent a patch. Maybe, we could show the error to only administrator users on the site in order to follow @alexvorn2 suggestion.

@umangvaghela123
2 years ago

Fix #41081 issue.

#3 follow-up: @mdifelice
2 years ago

Hi @umangvaghela123, about your patch: if you call wp_get_nav_menus() without arguments and it does not return anything, when you call it again it would return nothing again, does not it?

#4 in reply to: ↑ 3 @alexvorn2
2 years ago

Replying to mdifelice:

Hi @umangvaghela123, about your patch: if you call wp_get_nav_menus() without arguments and it does not return anything, when you call it again it would return nothing again, does not it?

He patch seems not good, for me.

#5 @welcher
2 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

@umangvaghela123 thanks for the patch! However, I think that calling wp_get_nav_menus() twice is not needed.

@mdifelice your patch looks correct to me - can you regenerate it from the root of the repository? It makes it easier to apply and if we need to add any unit tests, they can be included in the same patch.

In regards to showing the error to admins, I don't think it's required.

@mdifelice
2 years ago

Patch for hiding empty menus in Nav Menu Widget (differential from the root repository)

#6 follow-up: @mdifelice
2 years ago

Hi @welcher. Just sent it. I am pretty new with contributing with the WordPress core, so sorry if I do not follow certain conventions. I was not sure if using the filename or ticket number for the attachment, finally I used the ticket number since I saw a lot of other attachments that do it that way.

#7 in reply to: ↑ 6 @welcher
2 years ago

  • Keywords commit added; needs-refresh removed
  • Milestone changed from Awaiting Review to 4.9

Replying to mdifelice:

Hi @welcher. Just sent it. I am pretty new with contributing with the WordPress core, so sorry if I do not follow certain conventions. I was not sure if using the filename or ticket number for the attachment, finally I used the ticket number since I saw a lot of other attachments that do it that way.

@mdifelice no worries at all! Thank you for taking the time to contribute - we all have to start somewhere! It looks like you generated it from the /src dir. If we generate from one level higher, we'll get any changes to the /tests folder ( and other items ) as well.

I've added some updates to the messaging when the there are no menus found to better reflect the changes in this patch. Unit tests pass with this patch.

I think we're good to go!

@welcher
2 years ago

Updating messaging to reflect the changes in this patch.

#8 @mdifelice
2 years ago

Great @welcher. Perfect!

I have added a little modification on a typo in the description.

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

@mdifelice
2 years ago

Solved typo on new message.

#9 @westonruter
2 years ago

  • Keywords needs-patch added; has-patch commit removed

I don't think the approach here is complete. In the Customizer you can add new nav menu items to empty nav menus. A menu may be empty the moment you open the Customizer, and at that point the menu dropdown would skip listing it with this patch. But as soon as I add an item to that nav menu, the menu should start to appear in the dropdown, but that is not currently how the patch behaves. This will likely require JS that interfaces with the menus component in the Customizer.

This ticket was mentioned in Slack in #core-customize by welcher. View the logs.


2 years ago

#11 @mdifelice
2 years ago

In that case, the desired behavior would be to refresh all the current menu widgets whenever a menu is edited to leave only those with items in the dropdown. But what would happen if I delete all the items from a menu, should I delete it from the widget too?

Taking all this into account, I think that this change should be discarded since it would generate unnecessary complications.

#13 in reply to: ↑ 12 @alexvorn2
2 years ago

Replying to westonruter:

@mdifelice this is actually already accounted for in the Nav Menus component of the Customizer, specifically when nav menus get renamed, added, or deleted. See:

I have a custom menu widget, Is it possible to update my custom widget when a new menu is added/removed/renamed? I would much appreciate if you could answer me with a solution. (Sorry for asking help in this forum)

#14 @DrewAPicture
2 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to mdifelice
  • Status changed from new to assigned

Assigning ownership to mark the good-first-bug as "claimed".

#15 follow-up: @mdifelice
2 years ago

In this new patch I address adding or removing a menu when it has one (or more) or zero menu items respectively in the Customizer.

Watching the code I observed that this section is a little buggy, for instance when you add a menu before opening a Menu Widget (one Menu widget already inserted in a sidebar) that Menu Widget is not updated. That behavior occurred before the patch I submit, and it continues happening. It should be addressed in a next patch but before I wanted to know if someone had already observed it.

Maybe we can apply this patch and solve that in another ticket.

Also, anybody knows why the Menu Widget update is being made by the Menu Controller? I think it would be more maintainable if that update is made by the Widget Controller communicating them via events or something like that.

And about your question @alexvorn2 you may do your custom widget compatible with the Customizer, but I recommend you to use the WordPress built-in widget.

@mdifelice
2 years ago

New patch with Customizer support.

#16 in reply to: ↑ 15 ; follow-up: @alexvorn2
2 years ago

Replying to mdifelice:

Also, anybody knows why the Menu Widget update is being made by the Menu Controller? I think it would be more maintainable if that update is made by the Widget Controller communicating them via events or something like that.

  1. We can make the widget update after the saving #41103 (ticket closed but fell free to reopen)
  2. So after a new menu is added/removed/edited -> we just make the widget to save and it will update itself.

#17 in reply to: ↑ 16 @mdifelice
2 years ago

Replying to alexvorn2:

Replying to mdifelice:

Also, anybody knows why the Menu Widget update is being made by the Menu Controller? I think it would be more maintainable if that update is made by the Widget Controller communicating them via events or something like that.

  1. We can make the widget update after the saving #41103 (ticket closed but fell free to reopen)
  2. So after a new menu is added/removed/edited -> we just make the widget to save and it will update itself.

I think widgets are updating well, there could be a few bugs but it is working that way.

#18 @mdifelice
2 years ago

  • Keywords 2nd-opinion added

#19 @jbpaul17
2 years ago

  • Keywords needs-testing added

#20 @melchoyce
2 years ago

  • Keywords needs-screenshots added

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


2 years ago

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


2 years ago

#23 @jbpaul17
2 years ago

  • Milestone changed from 4.9 to Future Release

Punting to Future Release per today's 4.8 bug scrub.

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


11 months ago

#25 @audrasjb
8 months ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.