refactor: wizards menu order handling by miguelpeixe · Pull Request #3501 · Automattic/newspack-plugin

miguelpeixe · 2024-10-25T17:04:07Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

I started investigating the different ways we're managing menu order for wizard, prompted by @ronchambers comment regarding the Newsletters' wizard menu order:

I was having a hard time setting the Newsletters CPT parent menu item to a decimal value. It seems that CPTs only support an integer value for menu_positon. So I created a function that will loop through global $menu and then move the CPT to position 3.3 or "3.33" if something is already at 3.3. See the current code in function move_cpt_menu(). You may want to look at function registered_post_type_newsletters( $post_type ) to see how I'm changing the menu_icon but when I try to change the menu_position to 3.3 in that function it doesn't work.

Turns out we have multiple strategies in place:

This is hard to maintain and unpredictable. In this refactor PR I propose a consolidated strategy applied in the class-wizards.php:

newspack-plugin/includes/class-wizards.php

Lines 141 to 172 in 406bc54

    
           	/** 
        
           	 * Update menu order for wizards with parent menu items. 
        
           	 * 
        
           	 * @param array $menu_order The current menu order. 
        
           	 * 
        
           	 * @return array The updated menu order. 
        
           	 */ 
        
           	public static function menu_order( $menu_order ) { 
        
           		$index = array_search( 'newspack-dashboard', $menu_order, true ); 
        
           		if ( false === $index ) { 
        
           			return $menu_order; 
        
           		} 
        
           		$ordered_wizards = []; 
        
           		foreach ( self::$wizards as $slug => $wizard ) { 
        
           			if ( ! empty( $wizard->parent_menu ) && ! empty( $wizard->menu_order ) ) { 
        
           				$ordered_wizards[ $wizard->menu_order ] = $wizard->parent_menu; 
        
           			} 
        
           		} 
        
           		if ( empty( $ordered_wizards ) ) { 
        
           			return $menu_order; 
        
           		} 
        
           		ksort( $ordered_wizards ); 
        
           		foreach ( array_reverse( $ordered_wizards ) as $menu_item ) { 
        
           			$key = array_search( $menu_item, $menu_order, true ); 
        
           			if ( false === $key ) { 
        
           				continue; 
        
           			} 
        
           			array_splice( $menu_order, $key, 1 ); 
        
           			array_splice( $menu_order, $index + 1, 0, $menu_item ); 
        
           		} 
        
           		return $menu_order; 
        
           	}

Each wizard with a parent menu item can use the public vars parent_menu and menu_order to determine how it should render relative to the newspack-dashboard wizard:

newspack-plugin/includes/wizards/class-network-wizard.php

Lines 33 to 45 in 406bc54

    
           	/** 
        
           	 * The parent menu item name. 
        
           	 * 
        
           	 * @var string 
        
           	 */ 
        
           	public $parent_menu = 'newspack-network'; 
        
           	/** 
        
           	 * Order relative to the Newspack Dashboard menu item. 
        
           	 * 
        
           	 * @var int 
        
           	 */ 
        
           	public $menu_order = 5;

Result:

This PR also deprecates the menu_priority variable from the abstract class. It's a bit misleading, as it only applies the hook priority to the admin_menu hook. This is not a guaranteed way of setting the menu order because it gets overridden by the add_menu_page argument.

How to test the changes in this Pull Request:

Checkout this PR, make sure you have Ads, Network, and Newsletters plugin active
Confirm the menu order reflects the image above
Click each menu and confirm it behaves as expected

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

jaredrethman

@miguelpeixe have tested the code and works as described.

Additionally: I really like your approach, first time i’ve come across custom_menu_order hook - looks like the right tool for the job!

Approved!

miguelpeixe · 2024-10-25T18:16:58Z

Thanks, @jaredrethman! Since this touches on a core aspect of the project, I'll also wait for @ronchambers feedback before merging.

leogermani · 2024-10-25T18:24:06Z

Not sure if this has any impact here, but for awareness: https://core.trac.wordpress.org/ticket/52035

I've been meaning to circle back to this and help it get to the finish line

ronchambers · 2024-10-25T18:26:02Z

I didn't test locally, but I trust the two of you 😃 Overall, I agree that moving the menus around for the 3 different plugins (Listings, Newsletters, and Network - my original tasks ) became cumbersome where each plugin had a different approach... So yes, a standard approach is great! Since Listings and Newsletters were re-assigned to @miguelpeixe I won't worry about testing those. I do need to do some final testing on the Network plugin wizard so I'll be sure to merge this (or epic/ia after it's merged) to my Network branch and verify it's all good there too. Approved for me! Thanks!

miguelpeixe · 2024-10-25T18:35:50Z

Not sure if this has any impact here, but for awareness: https://core.trac.wordpress.org/ticket/52035

Thanks for flagging! The strategy doesn't cover submenu pages. I'm assuming we have tighter control over the execution order for submenu pages so it's less problematic.

refactor: wizards menu order handling

406bc54

miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label Oct 25, 2024

miguelpeixe self-assigned this Oct 25, 2024

miguelpeixe requested a review from a team as a code owner October 25, 2024 17:04

jaredrethman approved these changes Oct 25, 2024

View reviewed changes

github-actions Bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Oct 25, 2024

miguelpeixe requested a review from ronchambers October 25, 2024 18:17

miguelpeixe merged commit 435cf72 into feat/ia-newsletters Oct 25, 2024

miguelpeixe deleted the refactor/ia-wizard-menu-order branch October 25, 2024 18:36

miguelpeixe mentioned this pull request Oct 29, 2024

Feat / IA for Newsletters #3490

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: wizards menu order handling#3501

refactor: wizards menu order handling#3501
miguelpeixe merged 1 commit into
feat/ia-newslettersfrom
refactor/ia-wizard-menu-order

miguelpeixe commented Oct 25, 2024 •

edited

Loading

Uh oh!

jaredrethman left a comment

Uh oh!

miguelpeixe commented Oct 25, 2024

Uh oh!

leogermani commented Oct 25, 2024

Uh oh!

ronchambers commented Oct 25, 2024

Uh oh!

miguelpeixe commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/**
	* Update menu order for wizards with parent menu items.
	*
	* @param array $menu_order The current menu order.
	*
	* @return array The updated menu order.
	*/
	public static function menu_order( $menu_order ) {
	$index = array_search( 'newspack-dashboard', $menu_order, true );
	if ( false === $index ) {
	return $menu_order;
	}
	$ordered_wizards = [];
	foreach ( self::$wizards as $slug => $wizard ) {
	if ( ! empty( $wizard->parent_menu ) && ! empty( $wizard->menu_order ) ) {
	$ordered_wizards[ $wizard->menu_order ] = $wizard->parent_menu;
	}
	}
	if ( empty( $ordered_wizards ) ) {
	return $menu_order;
	}
	ksort( $ordered_wizards );
	foreach ( array_reverse( $ordered_wizards ) as $menu_item ) {
	$key = array_search( $menu_item, $menu_order, true );
	if ( false === $key ) {
	continue;
	}
	array_splice( $menu_order, $key, 1 );
	array_splice( $menu_order, $index + 1, 0, $menu_item );
	}
	return $menu_order;
	}

	/**
	* The parent menu item name.
	*
	* @var string
	*/
	public $parent_menu = 'newspack-network';

	/**
	* Order relative to the Newspack Dashboard menu item.
	*
	* @var int
	*/
	public $menu_order = 5;

Conversation

miguelpeixe commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Uh oh!

jaredrethman left a comment

Choose a reason for hiding this comment

Uh oh!

miguelpeixe commented Oct 25, 2024

Uh oh!

leogermani commented Oct 25, 2024

Uh oh!

ronchambers commented Oct 25, 2024

Uh oh!

miguelpeixe commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miguelpeixe commented Oct 25, 2024 •

edited

Loading