-
Notifications
You must be signed in to change notification settings - Fork 9
Fix title breadcrumbs in WC 5.8 #224
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
Conversation
instead of global `window.wcSettings` or settings store `getSetting`. Improves WC5.8 compatibility. Note, that the package is not published and not added to dependencies. Works around woocommerce/woocommerce-admin#7781
Thanks @tomalec ! |
to reduce the technical debt.
The WC 5.6-5.8 compatible way to obtain const woocommerceTranslation = wc.wcSettings.getSetting('woocommerceTranslation') || wc.wcSettings.getSetting('admin')?.woocommerceTranslation; It seems that It's not |
Mention findings from woocommerce/woocommerce-admin#7810 the unobvious source of unpublished `@woocommerce/settings` -> https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/assets/js/settings/shared/index.ts
@eason9487 would you mind giving this one a review too please? |
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.
Tested well. LGTM.
Changes proposed in this pull request
Use
@woocommerce/wc-admin-settings
@woocommerce/settings
imported via DEWP,instead of global
window.wcSettings
or settings storegetSetting
.Improves WC5.8 compatibility.
Note, that the package is not published and not added to dependencies.
Works around
countries
is missing from settings store,woocommerceTranslation
fromwcSettings
in 5.8.0-rc.1 woocommerce-admin#7781It also updates DEWP to the latest version, it is not required for the fix.
Screenshots
Detailed test instructions
npm install
npm run build
/wp-admin/admin.php?page=wc-admin&path=%2Fpinterest%2Fonboarding
document.title
in consoleYou should see
instead of
Changelog note
Todo
false
Additional notes
This PR introduces a dependency that is not mentioned in
package.json
which may break local unit tests in the future, but at least it somewhat indicates where thewcSettings
global comes from. Still, behind DEWP mystery, but at least "somewhat".