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
Adding test name block with support for entity data store #37132
Conversation
Test Results SummaryCommit SHA: ca24f9e
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #37132 +/- ##
========================================
Coverage 46.7% 46.7%
Complexity 17191 17191
========================================
Files 429 429
Lines 64845 64845
========================================
Hits 30275 30275
Misses 34570 34570
|
200cb32
to
78b920d
Compare
"compilerOptions": { | ||
"outDir": "build" | ||
"outDir": "build", | ||
"resolveJsonModule": true, |
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.
Need to tweak TS configuration to support importing JSON files, as there are benefits to using the standard block.json
file to provide metadata.
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.
+1
|
||
blocks.push( | ||
createBlock( nameBlock.name, { | ||
name: product.name, |
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.
Just curious if this logic will still be needed if we start using templates to generate the form?
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.
@louwie17 After our conversation this morning, my thought is no (it's not needed). I believe once we have your template PR merged I can just register this block and move it to the template..? If you get that merged first, I'm glad to rebase and retool a bit. Otherwise we can migrate it in that PR or another follow-up.
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.
Also in favor of removing this. Seems like at best we can avoid this, but worst case is we will need to parse the product and retrofit it into the hydrated template.
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.
Louren's settings PR is now merged, and I've rebased this PR to include it. I started experimenting with loading the blocks from the post type template in d284e9b. I'm uncertain of the pattern here, and arguably this might belong in a separate PR. I can continue on Monday but let me know if you have any thoughts @louwie17
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.
Hmm I don't think its necessary to register the template as a block (unless you saw a similar pattern in Gutenberg somewhere?)
In theory we should just be able to pass it into the setupEditor
function similar to what Josh started doing here: https://github.com/woocommerce/woocommerce/pull/36921/files#diff-5fbb147f0c06b95b81afa9f70ae6db13e289b2714f4b21d041a7bd88670a1d03R106
Josh's approach is similar to Gutenberg's approach here: https://github.com/WordPress/gutenberg/blob/trunk/packages/editor/src/components/provider/index.js#L84
We may also want to make use of the useBlockEditorSettings
that is being made use of there (if the call to setupEditor
is not enough).
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.
But on second thought it might help to leave this as is for now and merge it, to un-block any block field work and create a separate PR for the template
support.
6efc313
to
947b3f1
Compare
947b3f1
to
d284e9b
Compare
@joelclimbsthings I hope you don't mind, but I messed around with the template work and got it working in this commit: aff7ac2 |
fdca43d
to
ca24f9e
Compare
@joelclimbsthings and @louwie17 Good work here and also tested well to me. I'm going to merge this PR as we talked @louwie17 so we can unblock the rest of the issues the depend on this PR. |
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.
Good job!!!
All Submissions:
Changes proposed in this Pull Request:
Adding a basic name block to the product editor, with support for the entity data store.
Closes #37007 .
How to test the changes in this Pull Request:
new-product-management-experience
feature flag.core
store (for some reason this is listed twice for me, and I need to select the 2nd one)EDIT_ENTITY_RECORD
action is fired.Other information:
pnpm --filter=<project> changelog add
?FOR PR REVIEWER ONLY: