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

Fix/wizard admin menu priority 3 #3544

Merged
merged 14 commits into from
Nov 14, 2024
Merged

Conversation

ronchambers
Copy link
Collaborator

@ronchambers ronchambers commented Nov 13, 2024

All Submissions:

Changes proposed in this Pull Request:

The original wizards code in trunk gave us the ability to adjust the priority of a wizard's add_page callback. Somewhere in the timeline of the epic/ia development, this ability was hard-code then removed around the time we were fixing the parent menu custom ordering. In the end, after attempt 1 and attempt 2, and lots of discussion, testing and a huddle with @miguelpeixe - we've settled on this PR in order to reinstate the removed code.

Changes:

  • The parent menu order variable was renamed to be more descriptive of what it does: $parent_menu_order.
  • The reinstated variable for add page priority was give a descriptive name: $add_page_priority.
  • The old/hard-coded newspack parent menu was changed to $this->parent_menu so wizards can control which parent menu they are added to.
  • Some minimal changes were made to the advertising wizards and the dashboard.

How to test the changes in this Pull Request:

  1. Pull this branch on top of epic/ia.
  2. Run npm run build if you haven't already for epic/ia.
  3. Verify the wp-admin menus are displaying as expected.

parent-order

advertising

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?

@ronchambers ronchambers changed the base branch from trunk to epic/ia November 13, 2024 03:50
@ronchambers ronchambers marked this pull request as ready for review November 13, 2024 20:03
@ronchambers ronchambers requested a review from a team as a code owner November 13, 2024 20:03
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the discussions and tackling this! It's a nice and comprehensive solution. I left a non-blocking suggestion there, feel free to merge if you disagree.

Comment on lines 63 to 71
/**
* Priority for when 'add_page' callback fires. Default is WordPress's default hook priority of 10
* Override this on a per-wizard basis if there is a need to adjust the order that 'add_page' fires.
* Example: set priority < 10 so add_page is called before other pages. Or > 10 so it's called after other
* pages or plugins are loaded.
*
* @var int.
*/
protected $add_page_priority = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we thought of submenu_priority here, but I like this idea better. Following the same logic, the priority is applied to the admin_menu hook, so perhaps it should be called admin_menu_priority. WDYT? This is non-blocking, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I kept going back and forth... $admin_menu_priority is closer to what I had in original PR. Updated.

@github-actions github-actions bot added the [Status] Approved The pull request has been reviewed and is ready to merge label Nov 14, 2024
@ronchambers ronchambers merged commit f97a88a into epic/ia Nov 14, 2024
8 checks passed
@ronchambers ronchambers deleted the fix/wizard-admin-menu-priority-3 branch November 14, 2024 17:03
@ronchambers
Copy link
Collaborator Author

ronchambers commented Nov 14, 2024

@jaredrethman FYI. If you notice any issues with advertising/sponsors let me know. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants