-
Notifications
You must be signed in to change notification settings - Fork 21
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
WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087
Changes from 5 commits
d376e01
f325aa1
841cb94
d013eaa
d13cdf6
0468a73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
["@woocommerce/components","@woocommerce/currency","@woocommerce/customer-effort-score","@woocommerce/data","@woocommerce/date","@woocommerce/navigation","@woocommerce/number","@woocommerce/settings","@woocommerce/tracks","@wordpress/api-fetch","@wordpress/components","@wordpress/compose","@wordpress/data","@wordpress/data-controls","@wordpress/date","@wordpress/dom","@wordpress/element","@wordpress/hooks","@wordpress/html-entities","@wordpress/i18n","@wordpress/primitives","@wordpress/url","lodash","react","react-dom"] | ||
["@woocommerce/components","@woocommerce/currency","@woocommerce/customer-effort-score","@woocommerce/data","@woocommerce/date","@woocommerce/navigation","@woocommerce/number","@woocommerce/settings","@woocommerce/tracks","@wordpress/api-fetch","@wordpress/components","@wordpress/compose","@wordpress/data","@wordpress/data-controls","@wordpress/date","@wordpress/dom","@wordpress/element","@wordpress/hooks","@wordpress/html-entities","@wordpress/i18n","@wordpress/primitives","@wordpress/url","jquery","lodash","react","react-dom"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,39 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import $ from 'jquery'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { glaData } from '.~/constants'; | ||
import './custom-inputs'; | ||
import './index.scss'; | ||
|
||
// Originally, this extension relied on a WooCommerce core processing to show or hide | ||
// the product data tab and meta box added to the product editing page. | ||
// | ||
// However, WooCommerce Subscriptions has an additional processing, which overrides | ||
// the associated processed result in WooCommerce core. | ||
// | ||
// Since there is no available way to continue to work around it with `show_if_{productType}` | ||
// or `hide_if_{productType}` CSS classes, a jQuery custom event dispatched by WooCommerce core | ||
// is used to handle show or hide them instead. | ||
// | ||
// See: | ||
// - https://github.com/woocommerce/google-listings-and-ads/issues/2086 | ||
// | ||
// Ref: | ||
// - https://github.com/woocommerce/woocommerce/blob/8.0.3/plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js#L204-L243 | ||
// - https://github.com/Automattic/woocommerce-subscriptions-core/blob/6.2.0/assets/js/admin/admin.js#L18-L88 | ||
// - https://github.com/woocommerce/woocommerce/blob/8.0.3/plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js#L130-L158 | ||
$( document ).on( | ||
'woocommerce-product-type-change', | ||
'body', | ||
( e, productType ) => { | ||
const shouldDisplay = | ||
glaData.applicableProductTypes.includes( productType ); | ||
|
||
$( '.gla_attributes_tab, .gla_meta_box' ).toggle( shouldDisplay ); | ||
} | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,13 @@ | |
namespace Automattic\WooCommerce\GoogleListingsAndAds\Assets; | ||
|
||
use Automattic\WooCommerce\GoogleListingsAndAds\Exception\InvalidAsset; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Registerable; | ||
|
||
/** | ||
* Class AssetsHandler | ||
* | ||
* @package Automattic\WooCommerce\GoogleListingsAndAds\Assets | ||
*/ | ||
final class AssetsHandler implements Registerable, AssetsHandlerInterface { | ||
final class AssetsHandler implements AssetsHandlerInterface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a bit confused about the renaming of the add > register functions. If I recall correctly that name was previously avoided because it implemnted the Registerable interface. But I guess since you removed that here there is no issue with using the function name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood it right, only when a class implements the The previous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification. Yes that's correct that it needs to be both a I looked at all the assets further and they all seem to be registered/enqueued at the right time even after these changes. So that all looks good. |
||
|
||
/** | ||
* Assets known to this asset handler. | ||
|
@@ -21,32 +20,24 @@ | |
private $assets = []; | ||
|
||
/** | ||
* Add a single asset to the asset handler. | ||
* Register a single asset. | ||
* | ||
* @param Asset $asset Asset to add. | ||
* @param Asset $asset Asset to register. | ||
*/ | ||
public function add( Asset $asset ): void { | ||
public function register( Asset $asset ): void { | ||
$this->validate_handle_not_exists( $asset->get_handle() ); | ||
$this->assets[ $asset->get_handle() ] = $asset; | ||
$asset->register(); | ||
} | ||
|
||
/** | ||
* Add multiple assets to the asset handler. | ||
* Register multiple assets. | ||
* | ||
* @param Asset[] $assets Array of assets to add. | ||
* @param Asset[] $assets Array of assets to register. | ||
*/ | ||
public function add_many( array $assets ): void { | ||
public function register_many( array $assets ): void { | ||
foreach ( $assets as $asset ) { | ||
$this->add( $asset ); | ||
} | ||
} | ||
|
||
/** | ||
* Register a service. | ||
*/ | ||
public function register(): void { | ||
foreach ( $this->assets as $asset ) { | ||
$asset->register(); | ||
$this->register( $asset ); | ||
} | ||
} | ||
|
||
|
@@ -57,8 +48,8 @@ | |
* | ||
* @throws InvalidAsset If the passed-in asset is not valid. | ||
* | ||
* @see AssetsHandlerInterface::add To add assets. | ||
* @see AssetsHandlerInterface::add_many To add multiple assets. | ||
* @see AssetsHandlerInterface::register To register assets. | ||
* @see AssetsHandlerInterface::register_many To register multiple assets. | ||
*/ | ||
public function enqueue( Asset $asset ): void { | ||
$this->enqueue_handle( $asset->get_handle() ); | ||
|
@@ -71,8 +62,8 @@ | |
* | ||
* @throws InvalidAsset If any of the passed-in assets are not valid. | ||
* | ||
* @see AssetsHandlerInterface::add To add assets. | ||
* @see AssetsHandlerInterface::add_many To add multiple assets. | ||
* @see AssetsHandlerInterface::register To register assets. | ||
* @see AssetsHandlerInterface::register_many To register multiple assets. | ||
*/ | ||
public function enqueue_many( array $assets ): void { | ||
foreach ( $assets as $asset ) { | ||
|
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.
Based on the conditions we won't ever be on a WC Admin registered page and a (traditional) product edit page, so it's ok to pass the data to two separate scripts. I wonder though if this will always remain the case, like with the new product editing experience. Because in that case we would be setting
glaData
twice on the page.If
applicableProductTypes
is all that's needed then why wouldn't we just pass that separately so we'd never be able to conflict: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.
Thanks for the suggestion.
It's uncertain if the new Product Edit page will have the case of setting
glaData
twice. Indeed, it will be better to avoid possible conflicts in advance. Separated the inline script withe different variable names in 0468a73.