Make Product Collection product-centric #9469
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/product-collection/inspector-controls/index.tsx
assets/js/blocks/product-collection/inspector-controls/keyword-control.tsx assets/js/blocks/product-template/edit.tsx assets/js/blocks/product-template/products-middleware.tsx assets/js/blocks/product-template/test/products-middleware.ts |
|
Size Change: +945 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
kmanijak
added
skip-changelog
|
Hi @kmanijak, great work! 🙌🏻 I noticed that on the editor side, the titles of the products are not showing correctly. In the screenshot below, you can see that all the titles are displayed as "Post Title" in the Product Collection block. Are you able to reproduce this issue? 
|
| templateCategory, | ||
| } ); | ||
|
|
||
| const { products, productsLoading } = useStoreProducts( finalQuery ); |
There was a problem hiding this comment.
Out of curiosity, is there any reason we want to move away from getEntityRecords? Because recently, I came across P2 pdToLP-Aj-p2 in which Darren mentioned that we should "Converge on using the entity data store for product data in the editor" 🙂
There was a problem hiding this comment.
This is to improve the efficiency of fetching the product data. By default getEntityRecords fetches the data about the post. To make it fetch the product data (like prices, images, etc.) it would require to use a middleware (probably writing one to redirect the fetch, which is not ideal). I made a short POC some time ago, available here: #9162.
Because of that, I think it makes sense to use useStoreProducts for now.
There was a problem hiding this comment.
I'd prefer us to implement the entity records store (with middleware) please. This will help us move towards the convergence of product block usage across multiple views (product editor, site editor, etc). It's a good opportunity to make this change.
I started experimenting with something that could replace the withProductDataContext HOC in the price block refinement PR that might be useful to extract out to its own file. While the new ProviderFromAPI component still implements useStoreProducts, I've already noted it as a place to derive the data from the entity data store (and it'd be a good place to optionally implement a data transformation layer perhaps).
That work is still in progress but sharing it in case its useful.
There was a problem hiding this comment.
I'd prefer us to implement the entity records store (with middleware) please. This will help us move towards the convergence of product block usage across multiple views (product editor, site editor, etc). It's a good opportunity to make this change.
Thanks for your input! In that case, let me start moving toward the entity records store. I was a bit skeptical after the previous attempt, but I was redirecting the request to the Store API, while maybe we could get the data from WooCommerce API (like in this example middleware) - is that what you have in mind or do you think to still keep using Store API underneath?
I started experimenting with something that could replace the withProductDataContext HOC in the price block refinement PR that might be useful to extract out to its own file. While the new ProviderFromAPI component still implements useStoreProducts, I've already noted it as a place to derive the data from the entity data store (and it'd be a good place to optionally implement a data transformation layer perhaps).
I'll definitely take a deeper look. I think that solution like that should be prepared to avoid data fetch in case the product is already provided downstream (like it's intended to be in case of Product Collection). What I mean is in the case of All Products and Products block, inner blocks are responsible to fetch the product data which is then shared through the context (withProductDataContext ) to avoid fetching duplication. While in Product Collection the idea was to fetch all of the product data at once and just pass it downstream.
There was a problem hiding this comment.
but I was redirecting the request to the Store API, while maybe we could get the data from WooCommerce API (like in this example middleware) - is that what you have in mind or do you think to still keep using Store API underneath?
Yup, that's what I was thinking of (use WooCommerce API). Since it's in the admin, it aligns better with permissions on the authed endpoint (and lays the future groundwork for potentially editing content in place). Of course, you'll have to be careful for any client code that might be loaded on the frontend (i.e. the All Products block - which would still need to use the unauthed Store API).
While in Product Collection the idea was to fetch all of the product data at once and just pass it downstream.
Yup, that's what I was thinking of for the implementation I linked to. If it worked well, I planned on putting it into a separate PR so that the provider is at a higher level in the block tree (i.e. the Products Collection block).
kmanijak
force-pushed
the
add/product-centric-product-collection
branch
from
981f677 to
39c06e3
Compare
In order to fix that:
Summary:
cc: @imanish003 - I'm leaving the PR in this shape, feel free to pick it up or wait with it until I'm back 🙏 |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
imanish003
removed
the
status: stale
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
kmanijak
changed the title
imanish003
removed
the
status: stale
price is already formatted, not in a lowest currency unit...price is already formatted, not in a lowest currency unit as in store API 🚀 This comment was generated by the automations bot based on a
|
get actual currency dataget actual currency data 🚀 This comment was generated by the automations bot based on a
|
Add category linkAdd category link 🚀 This comment was generated by the automations bot based on a
|
|
Closing this as it's stall. The topic will have to be revisited but not ATM. |
This PR changes the way Product Collection / Product Template fetches the data compared to Products block (with inner Post Template).
Product Template:
Remaining issue:
with-product-data-contextHOC is organized. There's a hook to fetch the product data and even though the product is provided and data from the hook will be ignored (check if statement here), it's still making a request. By default, it would make a request per each product separately, however thanks to the different data structure, theidhere, is not defined, hence there's just a single call made. This requires rethinking/refactoring, however, we need to keep in mind All Products and Products block (and potentially others) in which Product Elements are in use. Having said that, I decided to accept this downside for now.Note: changes in
src/BlockTypes/AddToCartForm.phpare not related to this PR - it's just lining improvement.Fixes woocommerce/woocommerce#42388
Accessibility
Screenshots
Comparison of requests in Editor (Filters applied: fetch requests, including "product" in path):
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
This PR speeds up the loading time of Product Collection in the Editor. You can see the number of requests is decreased drastically: