Separate legacy WC menu handling to a separate hook,

[tomalec]

Tastes best with #1031

Changes proposed in this Pull Request:

Separate legacy WC menu handling to a separate hook, from <MainTabNav>.

Simplify it to use window.wpNavMenuClassChange instead of custom CSS selectors hacking.
Use it in Settings sub-pages.

Addresses part of #1037.

After this change, any page can highlight the menu, even ones without <MainTabNav>

Screenshots:

Before

After

Detailed test instructions:

0. Make sure you have WC Navigation disabled
1. Go to Settings page /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
2. and its subpages:
   1. /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings&subpath=%2Freconnect-accounts`
   2. /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings%2Fedit-phone-number/https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings&subpath=%2Fedit-phone-number

Make sure the "Google Listings & Ads" is highlighted on the left menu.

Changelog entry

Tweak - refactored legacy WC menu highlighting effect.
Fix - Report tabs lose active state when changing chart.

Additional notes

1. I had a problem with naming. We have the current WC navigation and the new one. I used "Legacy" for the current one, and "WC Navigation" for the new one, but I'm still not happy with those. "New", "old", "legacy", and any time-related name are almost by definition not stable in time. Every "new" would eventually become "old" and the current "new" may at some point become "legacy" leaving no way to differentiate the two. Any ideas for something better?

[eason9487]

This method looks good. But I have two questions:

1. ❓ Although the active state of tabs already had a glitch (Report tabs lose active state when changing chart #621), the active state is completely disappeared in this PR.
2. ❔ I wonder if the issue that WC menu highlight disappears after repeatedly clicking on the same tab could be fixed by this method as well?
   

[tomalec]

2. I wonder if the issue that WC menu highlight disappears after repeatedly clicking on the same tab could be fixed by this method as well?

I got the same effect on develop. And to me, it seems to be related to the <Link> used in the sub-navigation #984.

WP-admin re-renders the menu on the navigation even, and we have no hook to react on that.

We could fix it my calling useLegacyMenuEffect in the reports navigation render. But Id' rather fix it in a wider scope of navigation-related issues #1037
or at least by changing reportKey to path, like we did for subpath and make the legacy menu handler simply react on any { path } = getQuery() change.

[tomalec]

1. Although the active state of tabs already had a glitch (Report tabs lose active state when changing chart #621), the active state is completely disappeared in this PR.

🤦‍♂️

This is a result of ignoring the scoping in WP functions

window.wpNavMenuClassChange = function ( page, url ) {
	Array.from( document.getElementsByClassName( 'current' ) ).forEach(
		function ( item ) {
			item.classList.remove( 'current' );
		}
	);

it removes current class from every element on the page.

wpNavMenuClassChange: … document.getElementsByClassName
To me, it's a fundamental bug on WP/WC end. We should either leave it as is not to maintain too much hacky code on our end. Or introduce the actual CSS boundary on our end.

[tomalec]

Created an issue for 1. at woocommerce/woocommerce-admin#7772

[tomalec]

I also fixed it in WC woocommerce/woocommerce-admin#7773
Now, the question is should we keep this bug, or keep maintaining a fork as workaround until, our lowest supported WC version is the one that hopefully would have that fix. (my guesstimate is WC-admin 2.11.x => WC totally unknown 6.1?)

//cc @jconroy

[eason9487]

Looks like woocommerce/woocommerce-admin#7773 can fix both issues within the #1038 (review). 💯

Now, the question is should we keep this bug, or keep maintaining a fork as workaround until, our lowest supported WC version is the one that hopefully would have that fix.

Finally found out where the .subsubsub .current style comes from.

• https://github.com/WordPress/wordpress-develop/blob/5.8.1/src/wp-admin/css/common.css#L448-L451
• https://github.com/WordPress/wordpress-develop/blob/5.8.1/src/wp-admin/css/common.css#L683-L685

Considering that 1) probably we still have a few months to wait for WC-admin to release the fixes and our L-2 version to be ready for bumping up, and 2) the cost looks small enough if we maintain a copy that has only 5 lines and has not been changed for 8 years. As a trade-off to exchange for fixing #621 early, I think it's okay.

[tomalec]

Considering that 1) probably we still have a few months to wait for WC-admin to release the fixes and our L-2 version to be ready for bumping up, and 2) the cost looks small enough if we maintain a copy that has only 5 lines and has not been changed for 8 years. As a trade-off to exchange for fixing #621 early, I think it's okay.

Good point. I duplicated the current's styles as gla-sub-nav__item--current's ones.

[eason9487]

Testing well. LGTM.

[tomalec]

I made an issue, to eventually remove the workaround: #1048.

and a follow up for the 2. above: #1049