-
Notifications
You must be signed in to change notification settings - Fork 144
Try: Lock top level items in navigation #5478
Conversation
); | ||
$menu_args = wp_parse_args( $args, $defaults ); | ||
|
||
if ( ! $menu_args['multiple'] ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
- 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?
src/Features/Analytics.php
Outdated
'parent' => 'woocommerce', | ||
'path' => '/customers', | ||
'order' => 50, | ||
'parent' => $navigation_enabled ? 'customers' : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate parent
property here
src/Features/Analytics.php
Outdated
), | ||
'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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'] ) { |
There was a problem hiding this comment.
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.
src/PageController.php
Outdated
@@ -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'] ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks for taking a look at this one, @psealock. I updated the |
That may be true, but the context is important. And I do think that directly overriding the What do you think about renaming the property to |
Great idea. I think |
Yes, "Customers" is a category
Yup, agree. Multiple payments are installed and activated on the eComm plan so it makes sense to be a category.
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? |
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 |
Closing in favour of #5513 |
This PR seeks to lock the top level items so that all extending plugins must choose a category.
This solves some of the complexity around the guardrails needed, but introduces a few questions ( /cc @jameskoster @elizaan36 @psealock @joelclimbsthings ):
Screenshots
Detailed test instructions: