-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat/40: declare product compatibility #72
Conversation
Noting that this work is blocked by woocommerce/woocommerce#45143 |
@Sidsector9 is this unblocked now that woocommerce/woocommerce#46741 is merged (and part of Woo 8.9.0)? |
@jeffpaul I'll go through the upstream thread and confirm if it unblocks. |
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.
Few minor things I've flagged.
Couple other questions:
- Are there any new E2E tests we want to / should add?
- If I'm following the dependencies here correctly, seems the work here relies on Woo 8.9, is that correct? If so, do we need to wait to release this until 8.9 becomes our minimum?
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.
Code looks good. Left some minor suggestions 👍
@Sidsector9 some minor suggestions to resolve and then we can move this to UAT. |
@@ -37,7 +37,7 @@ | |||
"@woocommerce/dependency-extraction-webpack-plugin": "^1.4.0", | |||
"@woocommerce/eslint-plugin": "^2.2.0", | |||
"@wordpress/env": "^10.0.0", | |||
"@wordpress/scripts": "^27.9.0", | |||
"@wordpress/scripts": "^24.6.0", |
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 downgrade is purposeful I assume?
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.
@jeffpaul yes, React version mismatch
if ( 'simple' === product_type && ! sku.length ) { | ||
setSkuError( | ||
sprintf( | ||
__( `Please add an SKU to sync %s with Square. The SKU must match the item's SKU in your Square account.`, 'woocommerce-square' ), |
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.
Please update to "Please add a SKU..."
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.
@jeffpaul although SKU starts with a consonant, I think it will still use an "an" as "SKU" is typically pronounced as "ess-kay-you," starting with a vowel sound ("ess").
/** | ||
* External dependencies | ||
*/ | ||
import { useWooBlockProps } from '@woocommerce/block-templates'; |
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.
Looks like eslint warning that we should be able to resolve here
Regression / Smoke Test Report ✅Tested with Archive File created via "php woorelease.phar build repo_URL" (Composer version 2.5.5, npm version 8.19.4, node version 16.20.0) Status- Working expected with Plugin Archive/Zip file same as fix specific branch. Testing Environment
Next Step- Ready to Merge 🚀 |
@Sidsector9 Approving the regression testing, as I have created a separate issue 👍 |
All Submissions:
Changes proposed in this Pull Request:
Closes #40 .
Depends on woocommerce/woocommerce#45143
Steps to test the changes in this Pull Request:
Changelog entry