Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Customize Menus #55

Merged
merged 24 commits into from Jul 6, 2016

Conversation

Projects
None yet
4 participants
@valendesigns
Copy link
Member

commented Jul 2, 2016

Fixes #2

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2016

@miina & @westonruter I'm thinking we may need to handle the transition to publish in this PR before it's ready for merge. Thought?

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2016

@valendesigns transition to publish in what way?

$manager->snapshot()->is_preview = true;

$menu_items = array();
$menu_items[] = $this->manager->value_as_wp_post_nav_menu_item(

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 4, 2016

Contributor

I think we can avoid duplicating the WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item() method here by doing:

$setting_id = sprintf( 'nav_menu_item[%d]', static::MENU_ID );
$nav_menu_item = new WP_Customize_Nav_Menu_Item_Setting( $this->customize_manager, $setting_id, array(
    value: array( /* ... */ )
) );
$menu_items[] = $nav_menu_item-> value_as_wp_post_nav_menu_item();
if ( preg_match( \WP_Customize_Nav_Menu_Item_Setting::ID_PATTERN, $setting_id, $matches ) ) {
if ( (int) $menu->term_id === (int) $item['nav_menu_term_id'] ) {
$item['post_id'] = intval( $matches['id'] );
$items[] = $this->value_as_wp_post_nav_menu_item( (object) $item );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 4, 2016

Contributor

I believe we can re-use WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item() here.

if ( preg_match( \WP_Customize_Nav_Menu_Item_Setting::ID_PATTERN, $setting_id, $matches ) ) {
    $wp_customize->register_dynamic_settings( array( $setting_id ) );
    $nav_menu_item_setting = $wp_customize->get_setting( $setting_id );
    if ( $nav_menu_item_setting && (int) $menu->term_id === (int) $item['nav_menu_term_id'] ) {
        $item['post_id'] = intval( $matches['id'] );
        $items[] = $nav_menu_item_setting->value_as_wp_post_nav_menu_item(); // <========
    }
}
* @param array $args An array of arguments used to retrieve menu item objects.
* @return array Array of menu items.
*/
function filter_wp_get_nav_menu_items( $items, $menu, $args ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 4, 2016

Contributor

I wonder why this is needed as opposed to WP_Customize_Nav_Menu_Item::preview() which adds its own filter handler of the same name? https://github.com/xwp/wordpress-develop/blob/266ffe1f35e577954817d453cd7b2d84778cb57c/src/wp-includes/customize/class-wp-customize-nav-menu-item-setting.php#L368-L480

This comment has been minimized.

Copy link
@miina

miina Jul 4, 2016

Contributor

The issue is that these filters are used only settings-specifically but the menus are initially loaded at this moment in WP_Customize_Nav_Menus:
https://github.com/xwp/wordpress-develop/blob/master/src/wp-includes/class-wp-customize-nav-menus.php#L510

Loading the Customize_Nav_Menu_Setting itself to use the filters would require being related to existing setting IDs which are unknown before loading the Snapshot data.

* @param array $menus Array of menus.
* @return array Modified array of menus.
*/
public function filter_wp_get_nav_menus( $menus ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 4, 2016

Contributor

I wonder why this is needed as opposed to what WP_Customize_Nav_Menu_Setting::preview() adds? https://github.com/xwp/wordpress-develop/blob/master/src/wp-includes/customize/class-wp-customize-nav-menu-setting.php#L204-L296

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2016

@valendesigns @miina I believe this will reduce the overall amount of code and re-use what is in WP_Customzie_Nav_Menus already: #56

With those changes merged into this PR, the resulting diff is much smaller: develop...bugfix/saving-menus-issue_3

I tried creating a new nav menu, assigning it to a nav menu location, and adding new nav menu items to it. I created the snapshot and loaded it on the frontend in both an authenticated and incognito window, and the snapshotted menu and nav menu items appeared as expected for me in both cases.

Please review.

westonruter added some commits Jul 5, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jul 5, 2016

Coverage Status

Coverage decreased (-1.6%) to 91.031% when pulling 310b365 on bugfix/saving-menus-issue_2 into cb45595 on develop.

@coveralls

This comment has been minimized.

Copy link

commented Jul 5, 2016

Coverage Status

Coverage decreased (-1.6%) to 91.031% when pulling 310b365 on bugfix/saving-menus-issue_2 into cb45595 on develop.

Preview nav menu settings early so that the sections and controls for…
… snapshot values will be added properly

westonruter added some commits Jul 6, 2016

Merge pull request #56 from xwp/bugfix/saving-menus-issue_3
Re-use methods in WP_Customize_Nav_Menus in favor of duplication
@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

I think this is finally ready! 😂

@coveralls

This comment has been minimized.

Copy link

commented Jul 6, 2016

Coverage Status

Coverage decreased (-0.6%) to 92.105% when pulling d5e0bbc on bugfix/saving-menus-issue_2 into cb45595 on develop.

@westonruter westonruter merged commit 5dd4ed9 into develop Jul 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the bugfix/saving-menus-issue_2 branch Jul 6, 2016

@westonruter westonruter modified the milestone: 0.5.0 Aug 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.