Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Try: Lock top level items in navigation #5478

Closed
wants to merge 8 commits into from

Conversation

joshuatf
Copy link
Contributor

@joshuatf joshuatf commented Oct 27, 2020

This PR seeks to lock the top level items so that all extending plugins must choose a category.

  • Creates default top-level items and categories
  • Hides categories without items on the frontend
  • Simplifies WCA nav item registration
  • Simplifies post type registration

This solves some of the complexity around the guardrails needed, but introduces a few questions ( /cc @jameskoster @elizaan36 @psealock @joelclimbsthings ):

  • Should "Customers" be a category with a menu item of "All Customers" instead of a link? What if plugins want to add an item here? (Note that I'm seeing a duplicate "Customers" nav item, but this is an issue also on main)
  • Similarly, I would think that "Payments" would also be a category, otherwise it feels a bit like favoritism with WC Pay being allowed to register a top-level nav item.
  • The current setup allows a nav item with the ID of "home" to be registered as the top menu item. This could be overridden and we could introduce WCA as the dependency to "lock" this item, but I would think that plugin authors would not intentionally and maliciously try to override this item.
  • As a note, if third party items are eventually separated into a "Plugins" section, they would need to re-register nav items there.

Screenshots

Screen Shot 2020-10-27 at 12 50 22 PM

Detailed test instructions:

  1. Enable the nav feature.
  2. Check that all nav items continue working as expected.
  3. Try to register a top-level item.
  4. Note items default to the "Settings" category if a valid one is not provided.

);
$menu_args = wp_parse_args( $args, $defaults );

if ( ! $menu_args['multiple'] ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's probably a better name for this property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the significance of multiple? I see it used for shop_order but not understanding why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to create multiple menu items for the post type. Adding All {post_type}s and Add new {post_type} instead of just a single menu item with {post_type}.

Maybe multiple_labels is a better name, but open to feedback here.

Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Nice investigation here @joshuatf. This is working for me and I added a few code related questions.

I used my trusty example repo to give this a test from an extension's point of view. As advertised if I supplied no parent or 'parent' => 'woocommerce', then items did not appear. When specifying analytics or payments I was able to see the items.

Things we'd need to do for wc-admin extended pages

This approach would require a parent property from wc-admin extensions. See WC Pay using wc_admin_register_page here without a parent property.

https://github.com/Automattic/woocommerce-payments/blob/f830f53609acbd742eabd56b03a14ed289d94f7b/includes/admin/class-wc-payments-admin.php#L59-L67

  • Update documentation of wc_admin_register_page with the new requirement and a list of available top level menu items.
  • Update examples.
  • Explain how to handle the duality of the Nav sometimes being present, and not others. This may be difficult. Would it be worthwhile to just introduce a new hook and eventually deprecate the old?

'parent' => 'woocommerce',
'path' => '/customers',
'order' => 50,
'parent' => $navigation_enabled ? 'customers' : null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicate parent property here

),
'yes' === get_option( 'woocommerce_manage_stock' ) ? array(
'id' => 'woocommerce-analytics-stock',
'title' => __( 'Stock', 'woocommerce-admin' ),
'parent' => 'woocommerce-analytics',
'path' => '/analytics/stock',
'order' => 100,
'parent' => 'analytics',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you'd need to add this by default, otherwise this changes the requirement for the filter woocommerce_analytics_report_menu_items that extensions use to add a report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made all pages registered under analytics default to the analytics category.

);
$menu_args = wp_parse_args( $args, $defaults );

if ( ! $menu_args['multiple'] ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the significance of multiple? I see it used for shop_order but not understanding why.

@@ -475,25 +477,20 @@ public function register_page( $options ) {
public static function add_nav_item( $options ) {
$navigation_enabled = Loader::is_feature_enabled( 'navigation' );

if ( ! $navigation_enabled ) {
if ( ! $navigation_enabled || ! $options['parent'] ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If no parent is supplied, then the item isn't registered. How does this work with WC Payments that have not specified a parent property? Would we ask that they make a change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this would require a change from WC Payments to set the category as payments. If we default items to the settings category, then in this case we would end up with multiple Payment menu items.

@joshuatf
Copy link
Contributor Author

Thanks for taking a look at this one, @psealock. I updated the parent property to category so there's less confusion ambiguity around what the property is used for.

@psealock
Copy link
Collaborator

I updated the parent property to category so there's less confusion ambiguity around what the property is used for.

hmmm, I think this might add confusion because parent and category are now distinct properties for the same idea in different contexts:

Screen Shot 2020-10-29 at 9 02 01 AM

@joshuatf
Copy link
Contributor Author

hmmm, I think this might add confusion because parent and category are now distinct properties for the same idea in different contexts:

That may be true, but the context is important. And I do think that directly overriding the parent property may not be a good idea since this is used to register the original WP menu item. If JS is disabled in a browser, the fallback should still show the original items, but changing the parent would break this.

What do you think about renaming the property to navigation_parent or maybe incuding nav_args that's an array housing any wc nav specific properties?

@psealock
Copy link
Collaborator

What do you think about renaming the property to navigation_parent or maybe incuding nav_args that's an array housing any wc nav specific properties?

Great idea. I think navigation_parent or similar may be sufficient, but nav_args may be useful if we decide to programatically place items into buckets based on certain criteria.

@psealock
Copy link
Collaborator

psealock commented Nov 3, 2020

Should "Customers" be a category with a menu item of "All Customers" instead of a link? What if plugins want to add an item here? (Note that I'm seeing a duplicate "Customers" nav item, but this is an issue also on main)

Yes, "Customers" is a category

Similarly, I would think that "Payments" would also be a category, otherwise it feels a bit like favoritism with WC Pay being allowed to register a top-level nav item.

Yup, agree. Multiple payments are installed and activated on the eComm plan so it makes sense to be a category.

The current setup allows a nav item with the ID of "home" to be registered as the top menu item. This could be overridden and we could introduce WCA as the dependency to "lock" this item, but I would think that plugin authors would not intentionally and maliciously try to override this item.

Can we make a private method for this? I suppose this makes Navigation tied to wc-admin, which we wanted to avoid. Would it be a necessary evil?

@psealock
Copy link
Collaborator

psealock commented Nov 3, 2020

That may be true, but the context is important. And I do think that directly overriding the parent property may not be a good idea since this is used to register the original WP menu item. If JS is disabled in a browser, the fallback should still show the original items, but changing the parent would break this.

For non-wc-admin extensions, registering plugins inside of wc-calypso-bridge is fairly easy. But if we want to automatically register plugins into the Buckets system, it would be nice to keep the wc_admin_register_page the same. Can map common wc-admin extensions to categories in this repo as a temporary measure?

@psealock
Copy link
Collaborator

psealock commented Nov 4, 2020

Closing in favour of #5513

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants