Integration with new WC Navigation #605

ecgan · 2021-05-13T19:16:12Z

Changes proposed in this Pull Request:

Closes #285 .

This PR makes our GLA plugin work with the new WC Navigation experience.

Technical notes

In js/src/index.js, title seemed to be not used at all, I removed them in 3b577f5.

The WC Navigation menu item text depends on breadcrumbs (see code: https://github.com/woocommerce/woocommerce-admin/blob/7d70ebdd168eb91ccf0c6be4c7aec1ef7f07189d/client/layout/navigation.js#L68). Without changing the breadcrumbs, all the menu items show "Google Listings & Ads" (this was one of the major pains for me to figure out in this PR). So I had to fix the breadcrumbs, which was done in 706c8d8.

In the WC Navigation menu, when you click on menu items at the same hierarchy level, there will be menu transition animation, which does not match normal user experience. I believe this is an issue with WC Navigation. I looked into WC Payment https://github.com/Automattic/woocommerce-payments and their plugin is having the same issue as well.

Screen.Recording.2021-05-20.at.4.07.45.AM.mov

WC Navigation menu items are displayed based on path in the URL. If the path does not correspond to any items in the WC Navigation, then it will show the root WooCommerce level. This presents a problem for us in the report pages, because we have Programs Report at path=/google/reports/programs and Products Report at path=/google/reports/products, but we only have one "Reports" menu item in the WC Navigation, which I have tied to path=/google/reports/programs. This means that when we are displaying Programs Report, WC Navigation will display the menu item correctly, but when we navigate to Products Report, WC Navigation will show the root WooCommerce level. You can see this in the demo video below. To solve this problem, I'd suggest two ways:

Make the "Reports" a menu category, clicking on it will display another WC Navigation level with the two reports. This means we would need to ditch the Reports tab-based navigation. One problem with this during my experimentation is that the "Reports" menu category would appear at the very top above "Dashboard". I'm guessing there is some sort of logic to put menu category first followed by menu items.
Instead of using path=/google/reports/programs and path=/google/reports/products, we use another parameter to represent the type of report, e.g. path=/google/reports&report=programs and path=/google/reports&report=products. With this, the path always be path=/google/reports, and the WC Navigation menu item "Reports" will always show correctly. If this is ok, we can work on this in a separate issue.

Update 2021-05-25: I've created issue #665 to address the URL path issue.

Screenshots:

Demo video with my voice: https://d.pr/v/jLpVI2

wc-navigation-with-gla.mp4

Detailed test instructions:

Note: You can turn on/off the WC Navigation experience at https://gla1.test/wp-admin/admin.php?page=wc-settings&tab=advanced&section=features.

Turn on the WC Navigation experience.
When Setup MC is not completed yet, there should be only one Google Listings and Ads menu item, and clicking on it will display the Get Started page.
Click the Set up free listings button to start Setup MC flow. The Setup MC flow pages should display OK. (This uses the FullScreen component.)
When Setup MC is completed, the Google Listings and Ads menu would become a category. Clicking on it will show the next level of menu items.
Browse through the Dashboard, Reports, Product Feed, and Settings pages. They should display OK.
Go to Dashboard and click on the Edit button for Free Listings. The Edit Free Listings page should display OK. (This uses the FullContainer component.)
Turn off the WC Navigation experience and repeat the above steps.

Changelog Note:

Integrate GLA plugin with the new WC Navigation experience.

…item in php.

…NavigationItem.

Title seems to be not used at all. This has been cross checked in WC Admin code base.

Conflicts: js/src/reports/programs/index.js

…sitions.

Conflicts: js/src/reports/programs/index.js

jconroy · 2021-05-20T03:48:11Z

Awesome thanks @ecgan

eason9487

Some questions

❓. Not sure if this is the expected result. The main pages are not listed in the menu when adding/editing campaigns or editing the free listings.

📜 . Probably here is the pattern to consider having a HOC to conditional switching the new/old Navigation like:

import MainTabNav from './main-tab-nav';
import isWCNavigationEnabled from '.~/utils/isWCNavigationEnabled';

const withOldNav = ( WrappedPage ) => ( props ) => {
	return (
		<>
			{ ! isWCNavigationEnabled() && <MainTabNav /> }
			<WrappedPage { ...props } />
		</>
	);
};

Then it can wrap pages at the .~/index.js by

container: withOldNav( Dashboard ),

Or wrap pages individually at each page's index.js. Take the dashboard index.js as an example

export default withOldNav( Dashboard );

This way, it could avoid adding the same logic to every main page, and it would be easier to remove the old navigation supporting in the future.

js/src/reports/products/index.js

js/src/utils/isWCNavigationEnabled.js

src/Menu/Reports/Reports.php

js/src/index.js

Conflicts: js/src/product-feed/index.js js/src/reports/programs/index.js

…on WC Navigation.

ecgan · 2021-05-24T19:43:09Z

@eason9487 ,

Not sure if this is the expected result. The main pages are not listed in the menu when adding/editing campaigns or editing the free listings.

Yeah, this is actually related to the problem I mentioned in the PR description (I mentioned the case for Report pages, but it applies to the add campaign page, edit campaign page and edit free listings page like you have discovered).

The problem is: (quoted from PR description)

WC Navigation menu items are displayed based on path in the URL. If the path does not correspond to any items in the WC Navigation, then it will show the root WooCommerce level.

To fix this, we need to deal with the path, e.g. for edit free listings page, instead of using path=/google/edit-free-campaign, we use path=/google/dashboard&subpath=edit-free-campaign so that when edit free listings page is displayed, the Dashboard menu item is displayed and highlighted. We can think of it as edit free listings page is a child of dashboard page.

I'm thinking maybe we should work on these pages (reports pages and these add and edit pages) as a separate issue. I'll work on that after this PR is done.

Probably here is the pattern to consider having a HOC to conditional switching the new/old Navigation

That's a good catch, I was having a bit of funny feeling when I was doing import isWCNavigationEnabled and then coding the <MainTabNav> in the files.

For this case, I don't prefer to use HOC though. I usually use HOC for sharing logic or cross-cutting concerns (which has been replaced by hooks). Because we still want to render something (the MainTabNav) based on some conditional logic, I'd prefer it to be a component. I came up with a NavigationClassic component in 74133a8. With this approach, each page is able to dictate where the NavigationClassic shows up (e.g. we can wrap it in a div like in the Settings page), which would be more difficult to do if we use HOC.

ecgan · 2021-05-24T19:49:13Z

Also, I'm thinking maybe it's good to get some additional eyes on the PHP side of things (8 .php files changed in this PR). Maybe either one of @mikkamp @layoutd @nima-karimi could help out? 🙏 😄

eason9487

LGTM.

ecgan · 2021-05-25T12:14:04Z

@j111q , almost forgot, please see the PR description for the WC Navigation integration demo video. 🎥 😄

layoutd

💯 Looks good, two small notes.

layoutd · 2021-05-25T13:06:31Z

src/Menu/Dashboard.php

@@ -36,25 +37,57 @@ function( $menu_items ) {
 		add_action(
 			'admin_menu',
 			function() {


💅 This could all be moved to a separate function (similar to add_items in the woocommerce_marketing_menu_items hook).

layoutd · 2021-05-25T13:08:11Z

src/Menu/Dashboard.php

 			}
 		);
 	}

 	/**
-	 * Add Google Menu item under Marketing
+	 * Add Google Menu item under Marketing, when WC Navigation is not enabled.
 	 *
 	 * @param array $items
 	 *
 	 * @return array
 	 */
 	protected function add_items( array $items ): array {


💅 Since items are conditionally added, perhaps the function name should be updated to reflect that (maybe_...).

mikkamp

Looks good. Got similar comments as @layoutd but just sending them anyways.

mikkamp · 2021-05-25T13:07:40Z

src/Menu/Dashboard.php

+		if ( ! Features::is_enabled( 'navigation' ) ) {
+			$items[] = [
+				'id'         => 'google-dashboard',
+				'title'      => __( 'Google Listings & Ads', 'google-listings-and-ads' ),
+				'path'       => '/google/dashboard',
+				'capability' => 'manage_woocommerce',
+			];
+		}


Works fine this way, but for readability I would have refactored it slightly different:

Suggested change

if ( ! Features::is_enabled( 'navigation' ) ) {

$items[] = [

'id' => 'google-dashboard',

'title' => __( 'Google Listings & Ads', 'google-listings-and-ads' ),

'path' => '/google/dashboard',

'capability' => 'manage_woocommerce',

];

}

if ( Features::is_enabled( 'navigation' ) ) {

return $items;

}

$items[] = [

'id' => 'google-dashboard',

'title' => __( 'Google Listings & Ads', 'google-listings-and-ads' ),

'path' => '/google/dashboard',

'capability' => 'manage_woocommerce',

];

Actually, it's even better to move the condition a level higher (and leave the add_items function as is).

add_filter( 'woocommerce_marketing_menu_items', function( $menu_items ) { if ( Features::is_enabled( 'navigation' ) ) { return $items; } return $this->add_items( $menu_items ); } );

mikkamp · 2021-05-25T13:15:37Z

src/Menu/Dashboard.php

+				if ( ! Features::is_enabled( 'navigation' ) ) {
+					$this->fix_menu_paths();
+				} else {
+					wc_admin_register_page(


I'd move this functionality to a helper function so we can simplify the code in this action. Something like:

$this->register_navigation_pages();

7 lines of closing brackets in a row gets to be a bit much.

mikkamp · 2021-05-25T13:18:17Z

src/Menu/GetStarted.php

+				if ( ! Features::is_enabled( 'navigation' ) ) {
+					$this->fix_menu_paths();
+				} else {
+					wc_admin_register_page(


Same refactoring as suggested in the Dashboard page.

mikkamp · 2021-05-25T13:20:28Z

src/Menu/Reports/Reports.php

+			'admin_menu',
+			function() {
+				if ( apply_filters( 'gla_enable_reports', true ) ) {
+					wc_admin_register_page(


Here again I'd move this to a separate helper function to reduce the closing brackets (reached 8 in a row here).

mikkamp

The latest PHP changes look good, they are working well for me.

I just noticed that when I navigate from GLA > Reports > Programs to GLA > Reports > Products, the nav jumps back to WooCommerce Home, instead of staying on GLA.

ecgan · 2021-05-25T16:09:06Z

@mikkamp ,

I just noticed that when I navigate from GLA > Reports > Programs to GLA > Reports > Products, the nav jumps back to WooCommerce Home, instead of staying on GLA.

Yeah that is a known issue, I mentioned that in the PR description, in essence it is because:

WC Navigation menu items are displayed based on path in the URL. If the path does not correspond to any items in the WC Navigation, then it will show the root WooCommerce level.

I've created issue #665 to address this.

ecgan added 3 commits May 14, 2021 01:12

Fix TabNav effect breaking the new WC navigation.

67f7d3e

Add plugin item in new WC navigation.

24af863

Move new navigation menu to GetStarted and Dashboard php.

521bd15

ecgan self-assigned this May 13, 2021

ecgan added 20 commits May 14, 2021 18:47

Better simplified php code; also ran phpcbf command.

4bac7f0

Change menu item id in Dashboard.php.

5c6a2b2

Make client side working with WooNavigationItem.

023e1e3

Use wc_admin_register_page with nav_args instead of Menu::add_plugin_…

539df71

…item in php.

Use navArgs with fixed breadcrumbs instead of registerPlugin with Woo…

706c8d8

…NavigationItem.

Remove unneeded title.

3b577f5

Title seems to be not used at all. This has been cross checked in WC Admin code base.

Don't display TabNav if WC Navigation is enabled.

3ca31be

Better FullScreen code to make it work with WC Navigation.

dbbffa5

Merge branch 'trunk' into feature/new-navigation

b78a421

Conflicts: js/src/reports/programs/index.js

Fix page title: paid campaign, not free campaign.

5627309

CSS to make FullConatiner work with WC Navigation.

0570987

Code refactor with AppTabNav.

3eda79a

Remove unneeded initialName in TabNav instances.

e10b626

Logic to get selected tab from URL path.

0f0c43a

Change tab name to tab key.

ebf09a0

Make FullScreen work with WC Navigation with back and forth page tran…

6a92916

…sitions.

Rename TabNav to MainTabNav.

ddda24c

Display ReportsTabNav for Reports in WC Navigation.

1320acf

Add Reports php for navigation.

ec6af75

Make Reports work with WC Navigation.

e5afb24

Merge branch 'trunk' into feature/new-navigation

958d6a5

Conflicts: js/src/reports/programs/index.js

ecgan requested a review from a team May 19, 2021 19:52

ecgan changed the title ~~DRAFT: New Navigation~~ Integration with new WC Navigation May 19, 2021

eason9487 reviewed May 20, 2021

View reviewed changes

ecgan added 9 commits May 25, 2021 00:37

Merge branch 'trunk' into feature/new-navigation

7a05b39

Conflicts: js/src/product-feed/index.js js/src/reports/programs/index.js

Code refactor with NavigationClassic.

74133a8

Code refactor: use ReportsTabNav to render AppTabNav or SubNav based …

6833097

…on WC Navigation.

Rename ReportsTabNav to ReportsNavigation.

3fbea1b

Refactor: change SubNav into AppSubNav shared component.

ff0d7da

Make isWCNavigationEnabled not to return undefined.

cfddde2

Use isWCNavigationEnabled to check WC Navigation feature.

f652ac8

Fix undefined variable is_reports_enabled in Reports.php.

81465b1

Remove unneeded google-reports page.

a364c39

ecgan requested a review from eason9487 May 24, 2021 19:43

jconroy mentioned this pull request May 25, 2021

Error when viewing reports programs - summary section? #661

Closed

eason9487 approved these changes May 25, 2021

View reviewed changes

ecgan mentioned this pull request May 25, 2021

Modify path in URL to make additional pages work with WooCommerce Navigation #665

Closed

layoutd approved these changes May 25, 2021

View reviewed changes

mikkamp reviewed May 25, 2021

View reviewed changes

ecgan added 3 commits May 25, 2021 23:02

Refactor based on PR review comments on PHP.

a2ae6b6

Refactor and add feature check and fix filter.

f58c4c6

Fix WC Navigation causing full page reload for Reports page.

3a924d2

mikkamp approved these changes May 25, 2021

View reviewed changes

ecgan merged commit 93edefb into trunk May 25, 2021

ecgan deleted the feature/new-navigation branch May 25, 2021 16:09

ecgan mentioned this pull request Sep 30, 2021

RFC: Remove subpath URLQueryParam from settings pages. #1031

Open

3 tasks

This was referenced Sep 2, 2024

Remove Woo Navigation integration #2574

Merged

[Navigation Feature] Remove code strategically woocommerce/woocommerce#48997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration with new WC Navigation #605

Integration with new WC Navigation #605

ecgan commented May 13, 2021 •

edited

Loading

jconroy commented May 20, 2021

eason9487 left a comment •

edited

Loading

ecgan commented May 24, 2021

ecgan commented May 24, 2021

eason9487 left a comment

ecgan commented May 25, 2021

layoutd left a comment

layoutd May 25, 2021

layoutd May 25, 2021

mikkamp left a comment

mikkamp May 25, 2021

mikkamp May 25, 2021

mikkamp May 25, 2021

mikkamp May 25, 2021

mikkamp May 25, 2021

mikkamp left a comment

ecgan commented May 25, 2021

Integration with new WC Navigation #605

Integration with new WC Navigation #605

Conversation

ecgan commented May 13, 2021 • edited Loading

Changes proposed in this Pull Request:

Technical notes

Screenshots:

Detailed test instructions:

Changelog Note:

jconroy commented May 20, 2021

eason9487 left a comment • edited Loading

Choose a reason for hiding this comment

ecgan commented May 24, 2021

ecgan commented May 24, 2021

eason9487 left a comment

Choose a reason for hiding this comment

ecgan commented May 25, 2021

layoutd left a comment

Choose a reason for hiding this comment

layoutd May 25, 2021

Choose a reason for hiding this comment

layoutd May 25, 2021

Choose a reason for hiding this comment

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp May 25, 2021

Choose a reason for hiding this comment

mikkamp May 25, 2021

Choose a reason for hiding this comment

mikkamp May 25, 2021

Choose a reason for hiding this comment

mikkamp May 25, 2021

Choose a reason for hiding this comment

mikkamp May 25, 2021

Choose a reason for hiding this comment

mikkamp left a comment

Choose a reason for hiding this comment

ecgan commented May 25, 2021

ecgan commented May 13, 2021 •

edited

Loading

eason9487 left a comment •

edited

Loading