Custom Menu Widget should show newly added menus in Customizer

[adamsilverstein]

In the customizer, if you create a new menu it will not be available in the custom menu widget until you save the menu. A nice enhancement would be to allow you to assign newly created menus here.

Steps to reproduce:

1. Create a new menu in the Customizer
2. DON'T Save & Publish
3. Navigate to Widgets in Customizer
4. Add a Custom Menu widget
5. Dropdown for menu selection will not include the new menu

[adamsilverstein]

initial work on adding menus into custom menu widget on the fly

[adamsilverstein]

32814.diff​:
Is an incomplete attempt to insert newly created menus to the custom menu widget template on the fly. With this patch, if you a new menu (without saving), then switch to the widgets and insert the custom menu widget, the new menu will be available in the dropdown. However, the menu id is the placeholderId, and needs updating during save/publish.

[celloexpressions]

This is pretty important functionality - it needs to just work. Users won't think to save first, and may not want to publish changes until after adding the menu to a widget.

[adamsilverstein]

I've noticed another issue while working on this: despite the fact that the new menu is added to custom menu widget options, selecting it doesn't work. Instead, when i select a newly created menu I am seeing a console error: Uncaught TypeError: Cannot read property 'active' of undefined as well as an ajax callback failure.

The ajax callback is trying to get the menu data from the server (rendering the widget), and i believe its not there yet, so we likely need to either temporarily save it there, or better yet adjust the logic to recognize this is a new item, and use the local data.

The ajax request: ​http://cl.ly/image/3O2C2J1a2E42

in addition, I'm seeing PHP warnings in the preview when this callback occurs.

This hole keeps getting bigger the more i dig!

Thinking we should consider taking a step back and using a different approach. We could skip adding the menu to the widgets for now, instead adding a notice informing users in the widget itself that they need to save and publish the new menu before it will be available to the widget.

This solves all the issues in one step, and really doesn't reduce the functionality: the user can create and save a menu, the still non-destructively try assigning that menu item to a custom widget area before saving that change.

Feedback welcome :)

[tywayne]

Was playing around with a patch for this last night - hit the same console error and PHP warnings in the preview and gave up there.

The alternative you pitched with a notice telling users to save could work, but pains me to think about someone creating a new menu, navigating away to widgets and drilling in, only to find a notice telling them they have to save - as a user this would incite a short moment of panic, quickly trying to remember what all I've changed, and if it is ok to go ahead and publish.

Though, maybe I'm dramatizing that assumption? Is there precedent anywhere else in the customizer where we tell a user they have to save before they can continue?

Ideally I would push for making it work without having to publish first. As @celloexpressions mentioned above, users aren't going to think to hit save, and especially may not want to publish changes until seeing it - which is one of the major benefits to the customizer as a whole. Granted, I'm also not familiar enough with this to write such a patch :)

[samuelsidler]

Replying to tywayne:

Though, maybe I'm dramatizing that assumption? Is there precedent anywhere else in the customizer where we tell a user they have to save before they can continue?

No, there's not. I would hope there's some way we can work around this.

[adamsilverstein]

Replying to samuelsidler:

Replying to tywayne:

Though, maybe I'm dramatizing that assumption? Is there precedent anywhere else in the customizer where we tell a user they have to save before they can continue?

No, there's not. I would hope there's some way we can work around this.

There is a way forward, it is just complicated. I will continue working on this.

[adamsilverstein]

Still working on this item.

Looks like the best/most straightforward approach will be to save the new unpublished menu items to the server via ajax so that rendering can happen correctly for the custom menu widget (and any other customizer element referencing menus).

Since we previously had code that was saving the menus via ajax, I should be able to leverage this old code - working on that now. We will need to fire events to add, update or delete menus, using the temporary menu id. Once this is in place the existing widget rendering functionality should work as expected. Once the menu is 'saved and published' we will need to remove the temporary php side data and update any widgets to use the new ID (partially in place already), while also maintaining the correctly selected (potentially new) menus.

Replying to adamsilverstein:

Replying to samuelsidler:

Replying to tywayne:

Though, maybe I'm dramatizing that assumption? Is there precedent anywhere else in the customizer where we tell a user they have to save before they can continue?

No, there's not. I would hope there's some way we can work around this.

There is a way forward, it is just complicated. I will continue working on this.

[westonruter]

Replying to adamsilverstein:

Still working on this item.

Looks like the best/most straightforward approach will be to save the new unpublished menu items to the server via ajax so that rendering can happen correctly for the custom menu widget (and any other customizer element referencing menus).

Since we previously had code that was saving the menus via ajax, I should be able to leverage this old code - working on that now. We will need to fire events to add, update or delete menus, using the temporary menu id. Once this is in place the existing widget rendering functionality should work as expected. Once the menu is 'saved and published' we will need to remove the temporary php side data and update any widgets to use the new ID (partially in place already), while also maintaining the correctly selected (potentially new) menus.

I really do not think that persisting nav menu items via Ajax is the way to go here, as it's the whole point of the refactor work to ensure the Customizer state is fully captured in the settings currently in memory on the client.

When a new menu is created added, this menu should be added to the nav menu widgets with the placeholder ID supplied as the value. I think the part that is missing is that we need to take the currently-dirty settings as exposed in wp.customize.previewer.query().customized and pass this along with the update-widget Ajax request. This will ensure that all of the menus are available to the widget when it tries to call the update callback.

When the Customizer session is saved, then any widgets that reference the placeholder ID will need to have that menu option value updated to use the new permanent ID, and a change event triggered on the select to cause the update-widget call to happen again with the updated menu item. This is similar to what is being done now when the menus get updated after a save happens (their settings get updated and then their dirty states are reset). Now, the thing that is then missing here is that the nav menu widgets need to get updated to replace any reference in a widget instance to a placeholder ID to one that uses the permanent ID. This could either be done in a secondary re-save step at customize_save_after, or it could be done as part of the nav menu widget itself, to force the saving of any placeholder nav menus first so that it can then use the permanent ID when the widget is saved. This would be similar to how the logic for saving a nav menu item settings ensures that its parent nav menu setting is saved first.

[adamsilverstein]

continue custom menu widget integration

[adamsilverstein]

Posting 32814.2.diff​ (a work in progress) for feedback, review & testing:

• Includes duplicate dom manipulation code removal from other related ticket
• When adding a new (unsaved) menu to a custom menu widget, ensure that menu's data is sent as part of the widget-update ajax request
• On the back end, when a new menu id is being used for a custom menu widget, use the passed 'customized' data to generate the widget html
• Started working on code to refresh widgets when menu updated, not certain how to refresh control once I find a wigdet using the menu in question (I could find them, but not refresh); temp brute force workaround it to refresh all controls when menu changed - need some help fixing this
• Added code to refresh id when saved and published, so widgets using the saved menu get the new (real) id, needs testing
• Slight update to wp_get_nav_menu_object was required to avoid a php warning where get_term was being called with menu object

[adamsilverstein]

In 32814.3.diff​:

• Better custom menu widget refresh on menu change: only refresh widgets that contain the changed menu, including new menus; uses widgetControl.previewer.trigger( 'refresh' );
• clean up nav_menu_object to include all possible items

[adamsilverstein]

32814.4.diff​: added some isset checks for variables before using in filter_nav_menu_object

[adamsilverstein]

ensure newly saved menu remains selected

[adamsilverstein]

In 32814.5.diff​:

• When clicking Save & Publish, find any old temporary menu references in custom menu widgets, and remove them
• If a temporary menu was selected, reselect the newly saved item (new id)

Fixes an issue where if you created a menu, assigned it into a custom menu widget and then clicks save & publish, the menu was no longer selected.

Need to test: do we need to invalidate the widget and re-save?

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/60674ed62676c7748b2b98306235deb6c669f8a5 PR: ​https://github.com/xwp/wordpress-develop/pull/98

[westonruter]

@adamsilverstein I've done a review of the changes:

I think the addition of _resetCustomMenuWidgets will no longer be needed with the pending patch made to #32841. The menus rendered in Custom Menu widgets weren't getting properly refreshed, and this I believe I've resolved in the patch on that ticket. Merely changing a menu or menu item should be all that is needed to trigger a menu to refresh. Please try merging that patch into yours, and see if it works as expected without _resetCustomMenuWidgets being active.

I commented this on the ​PR, but when a Custom Menu widget is saved which had selected a new menu… I don't think this is currently persisting the newly-inserted menu ID in the widget settings. In other words, if you create a menu (without saving), assign it to a Custom Menu widget, and then Save… you have it update the UI to select the proper nav menu ID, but if you then reload the Customizer and re-examine the widget, it seems like it would still be trying to reference the placeholder nav menu?

I think the widget update needs to be handled on the server instead: when inserting a new nav menu, I think during the customize_save action we need to check if any Custom Menu widgets are referencing the placeholder ID, and then at that time make sure that all of these widgets get updated to reference the newly inserted term ID. And then the JS-value settings can be sent back in the customize_save_response filter: there can be a data.custom_menu_widget_updates just like we're including data.nav_menu_updates, and then for each data.custom_menu_widget_updates it can update the settings with those values, and then clear out the dirty state.

Lastly, I think that WP_Customize_Nav_Menus::filter_nav_menu_object() is duplicating the functionality of WP_Customize_Nav_Menu_Setting::preview(). I'm not clear why this is needed. Could you clarify?

[adamsilverstein]

Thanks for the feedback, I will work on those points. I'll merge in that patch and retest.

I missed the functionality in WP_Customize_Nav_Menu_Setting::preview() or it wasn't working for some reason, I will revisit.

[wonderboymusic]

I may blow up some patches here - going to code review wp.customize.menusPreview

[wonderboymusic]

reboot.diff​ makes this JS look more like the other Customizer JS. I am not going to drop it in right now. I may have to wait until all of the Customizer turbulence is over and then do one cleanup.

A few notes:

• The $.Deferred is unnecessary - when resolved, it just calls init, which adds another callback to the stack, which then gets called next. All of these can be combined into one.
• self is a bad local variable name. Sometimes self is a substitute for this when scope is lost in a maze of callbacks, but most of the time, it just makes this more ambiguous than it already is.
• Almost all of these functions are static helper methods, do any of them need to exposed publicly? We have gotten into a bad habit of globalizing all of our JS, which has made BC a living hell when we want to use things like browserify. I would expose as little as possible everywhere.
• We should distinguish between settings and class/instance properties.

[westonruter]

Replying to wonderboymusic:

reboot.diff​ makes this JS look more like the other Customizer JS. I am not going to drop it in right now. I may have to wait until all of the Customizer turbulence is over and then do one cleanup.

Right, is this the right place for such a general code review? Could you create a separate ticket for these points? I don't want the conversation here to sideline the work that adamsilverstein is doing here. And then we can discuss your notes on that ticket.

[wonderboymusic]

ignore for now - #32911

[adamsilverstein]

Working on the saving of new menus selected in custom menu widgets while the first part on #32841 gets resolved.

Replying to westonruter:

@adamsilverstein I've done a review of the changes:

I think the addition of _resetCustomMenuWidgets will no longer be needed with the pending patch made to #32841. The menus rendered in Custom Menu widgets weren't getting properly refreshed, and this I believe I've resolved in the patch on that ticket. Merely changing a menu or menu item should be all that is needed to trigger a menu to refresh. Please try merging that patch into yours, and see if it works as expected without _resetCustomMenuWidgets being active.

I commented this on the ​PR, but when a Custom Menu widget is saved which had selected a new menu… I don't think this is currently persisting the newly-inserted menu ID in the widget settings. In other words, if you create a menu (without saving), assign it to a Custom Menu widget, and then Save… you have it update the UI to select the proper nav menu ID, but if you then reload the Customizer and re-examine the widget, it seems like it would still be trying to reference the placeholder nav menu?

I think the widget update needs to be handled on the server instead: when inserting a new nav menu, I think during the customize_save action we need to check if any Custom Menu widgets are referencing the placeholder ID, and then at that time make sure that all of these widgets get updated to reference the newly inserted term ID. And then the JS-value settings can be sent back in the customize_save_response filter: there can be a data.custom_menu_widget_updates just like we're including data.nav_menu_updates, and then for each data.custom_menu_widget_updates it can update the settings with those values, and then clear out the dirty state.

Lastly, I think that WP_Customize_Nav_Menus::filter_nav_menu_object() is duplicating the functionality of WP_Customize_Nav_Menu_Setting::preview(). I'm not clear why this is needed. Could you clarify?

[adamsilverstein]

In 32814.7.diff​:

• remove _resetCustomMenuWidgets, verified no longer required
• fix issue where new menus not added to widget template so new menus missing from newly added custom menu widgets
• remove redundant filter_nav_menu_object function

[adamsilverstein]

A bit more cleanup in the latest patch. still don't have saving of new and selected menus working correctly,

Weston - I understand what needs to happen but may need help deciding exactly where to add the code and how to find the widgets we need. Specifically not sure where to hook into customize_save and once hooked how to identify Custom Menu widgets using the new id and update them. I see where the returned data should be handled and pretty much have that part covered in the old code it just needs to operate on the returned data.

[westonruter]

#32800 was marked as a duplicate.

[westonruter]

I'll be reviewing the patch and working to finish this out, as I noted earlier this week.

[obenland]

@westonruter, can we get that closed out before beta4?

[westonruter]

I will try to get this committed tomorrow before the dev meeting.

[westonruter]

Replying to westonruter:

I will try to get this committed tomorrow before the dev meeting.

Sorry, I meant to comment this on #32708. I haven't had time to dig into this, but I will have availability today and tomorrow to pick it up.

[westonruter]

In 32814.9.diff​:

• Dynamically update Custom Menu widget with appropriate messaging when initial menu is added/removed; remove menus from existing widgets and Custom Menu widget template: ​ddd65fe
• Fix section.active.validate() override method when section is removed: ​8cb85cc

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/a9238e11926b78cae5df87fecd7c86bdb75c85a2...c7508034cc1c2e648c46c30668633abf1d2ca181

[westonruter]

In 32814.10.diff​:

• Link to the menus Customizer panel from Custom Menu widget: ​524744e
• Add missing wp_get_nav_menus() hooks to preview Customizer updates/inserts for nav_menu settings: ​c750803

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/c750803...6a4d458

[westonruter]

In 32814.11.diff​:

• Make sure that other setting changes get previewed when updating a widget since it may depend on them: ​6a4d458
• Fix notice when get_current_screen() called in Ajax context for Menus panel deep link from Custom Menu widget: ​ba166fb

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/6a4d458...e971770e47

[westonruter]

In 32814.12.diff​:

• Fix updateWidget() calls from having to be aborted when changing select dropdown: ​e971770
• Update nav_menu widgets and widget template with newly-inserted menu ID: ​13fcb25

@adamsilverstein: OK. I think I've accounted for the various scenarios. Please test!

[adamsilverstein]

Testes the latest patch and this works really well! Most importantly, creating a a new menu, assigning it to a custom menu widget and saving now works perfectly! Yea, thank you @westonruter!

• created new menu
• added to existing custom menu widget
• added to new custom menu widget
• adjusted menu
• saved menu

also tested:

• delete new menu

All behaved as expected!

Only issue I see is some console errors when changing assigned menu: JQMIGRATE: jQuery.fn.attr('selected') may use property instead of attribute don't see use of .attr( in your patch, might be elsewhere.

[westonruter]

In 33488:

Customizer: Ensure that all existing menus are shown in the Custom Menu widget's dropdown.

• Ensure that a Custom Menu widget selecting a newly-inserted menu gets updated to use the new menu ID upon Save & Publish.
• Dynamically update the visibility of the Custom Menu widget's "no menus" message when the number of menus changes between 0 and 1+.
• Send all dirty Customized settings in update-widget Ajax request and preview() them so that the widget update/form callbacks have access to any data dependencies in the current Customizer session (such as newly created unsaved menus).
• Update link in Custom Menu widget to point to Menus panel as opposed to Menus admin page, when in the Customizer.
• Fix an issue with extra space at top immediately after creating new menu.
• Fix doubled update-widget Ajax requests when changing select dropdown; prevent initial from being aborted.
• Add missing wp_get_nav_menus() hooks to preview Customizer updates/inserts for nav_menu settings; includes tests.
• Update wp_get_nav_menu_object() to allow a menu object to be passed in (and thus passed through).

Props westonruter, adamsilverstein.
Fixes #32814.

[westonruter]

In 33496:

Customizer: Update nav_menu_locations[...] controls to remove placeholder menus from the dropdown options upon Save & Publish.

See #32814.
Fixes #33176.

[netweb]

Changeset r33488 broke PHP 5.2 Unit tests, Travis CI uses PHPUnit 3.6.12 for PHP 5.2

Failing Job: ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/73219883

Both the following tests use assertNotFalse() which is not included in PHPUnit 3.6.12

• Test_WP_Customize_Nav_Menu_Setting::test_preview_updated
  • /tests/phpunit/tests/customize/nav-menu-setting.php?annotate=blame#L218
• Test_WP_Customize_Nav_Menu_Setting::test_preview_inserted
  • /tests/phpunit/tests/customize/nav-menu-setting.php?annotate=blame#L263

[SergeyBiryukov]

In 33526:

Customizer: Fix failing tests on PHP 5.2 after [33488].

assertNotFalse() is not included in PHPUnit 3.6.12, use assertInternalType( 'int' ) instead.

fixes #32814.