Opened 7 years ago
Last modified 2 years 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: |
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)
Change History (32)
#2
@
7 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.
#3
follow-up:
↓ 4
@
7 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
@
7 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
@
7 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.
@
7 years ago
Patch for hiding empty menus in Nav Menu Widget (differential from the root repository)
#6
follow-up:
↓ 7
@
7 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
@
7 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!
#8
@
7 years ago
Great @welcher. Perfect!
I have added a little modification on a typo in the description.
#9
@
7 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.
7 years ago
#11
@
7 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.
#12
follow-up:
↓ 13
@
7 years ago
@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:
- https://github.com/WordPress/wordpress-develop/blob/ddc823a3e9039e6f8f2278cd5a34d9af125bf3df/src/wp-admin/js/customize-nav-menus.js#L2179-L2205
- https://github.com/WordPress/wordpress-develop/blob/ddc823a3e9039e6f8f2278cd5a34d9af125bf3df/src/wp-admin/js/customize-nav-menus.js#L2220-L2228
- https://github.com/WordPress/wordpress-develop/blob/ddc823a3e9039e6f8f2278cd5a34d9af125bf3df/src/wp-admin/js/customize-nav-menus.js#L2368-L2387
- https://github.com/WordPress/wordpress-develop/blob/ddc823a3e9039e6f8f2278cd5a34d9af125bf3df/src/wp-admin/js/customize-nav-menus.js#L2932-L2942
So the work here would be essentially to extend that to catch for when the number of items in a nav menu change between zero and not zero.
#13
in reply to:
↑ 12
@
7 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
@
7 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:
↓ 16
@
7 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.
#16
in reply to:
↑ 15
;
follow-up:
↓ 17
@
7 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.
- We can make the widget update after the saving #41103 (ticket closed but fell free to reopen)
- 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
@
7 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.
- We can make the widget update after the saving #41103 (ticket closed but fell free to reopen)
- 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.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#23
@
7 years ago
- Milestone changed from 4.9 to Future Release
Punting to Future Release per today's 4.8 bug scrub.
@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 theform()
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.