This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix: incorrect default number of products in editor when inheriting query #10303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
woocommercebot
requested review from
a team and
imanish003
and removed request for
a team
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +147 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
imanish003
suggested changes
Jul 24, 2023
There was a problem hiding this comment.
Hi @dinhtungdu,
Thanks for working on this. I was verifying the number of products as mentioned in the following table:
However, for the Product (Beta) block, the number of products is not correct in the Editor. It's showing 9 products instead of 12 in the editor. But, it displays the correct number of products on the frontend. I'm not sure if this is happening only for me. 🤷🏻♂️
| @@ -139,6 +139,7 @@ export const WooInheritToggleControl = ( | |||
| setQueryAttribute( props, { | |||
| ...props.defaultWooQueryParams, | |||
| inherit, | |||
| __woocommerceInherit: inherit, | |||
There was a problem hiding this comment.
AFAIK, we should use inherit instead of __woocommerceInherit. The __woocommerceInherit is added only when isCustomInheritGlobalQueryImplementationEnabled is true, but we've decided not to implement the custom inherit query from template. Therefore, this flag will always be false, as seen here. You can also find related discussion on Slack here: p1681812585287949-slack-C02UBB1EPEF.
In short, we shouldn’t use __woocommerceInherit. Additionally, I believe we can remove the isCustomInheritGlobalQueryImplementationEnabled flag and related code since we decided not to implement the "custom inherit query from template." I understand it might be confusing because there is still a partial implementation for the "custom inherit query from template," which is disabled by using the isCustomInheritGlobalQueryImplementationEnabled flag.
Does that make sense?
CC: @gigitux
There was a problem hiding this comment.
In short, we shouldn’t use __woocommerceInherit. Additionally, I believe we can remove the isCustomInheritGlobalQueryImplementationEnabled flag and related code since we decided not to implement the "custom inherit query from template." I understand it might be confusing because there is still a partial implementation for the "custom inherit query from template," which is disabled by using the isCustomInheritGlobalQueryImplementationEnabled flag.
Since the logic is added on the Products block (that will be deprecated) I agree with you, we can remove that logic!
There was a problem hiding this comment.
Thanks @imanish003 for the review, I agree this is a bit confusing, so let's discuss more.
- In 0d8fc18, I fixed the issue you mentioned in the review. I didn't handle well the case adding the block into archive template in the template editor.
- I use the
__woocommerceInheritattribute just for detecting if the current rest query is inherited from template. I used it in therest_product_queryfilter here (as a custom rest query argument). The reason for that is I can't find a better way, I'm open to any alternative that we can detect the 'inherit state' in the rest query. I admit this is a workaround than a solution, but given that theProducts (Beta)block will be deprecated, this is acceptable IMO. - About removing
isCustomInheritGlobalQueryImplementationEnabled, let's do it in a separated PR for better history.
There was a problem hiding this comment.
In 0d8fc18, I fixed the issue you mentioned in the review. I didn't handle well the case adding the block into archive template in the template editor.
Not sure why but it's still showing 9 products in Products beta block 🤔 Are you able to replicate this?
I use the __woocommerceInherit attribute just for detecting if the current rest query is inherited from template. I used it in the rest_product_query filter here (as a custom rest query argument). The reason for that is I can't find a better way, I'm open to any alternative that we can detect the 'inherit state' in the rest query. I admit this is a workaround than a solution, but given that the Products (Beta) block will be deprecated, this is acceptable IMO.
Got it. In that case, maybe we should consider naming this something other than __woocommerceInherit because its original intention was for use in the "custom inherit from query" implementation. Since this fix is not directly related to "custom inherit from query," using __woocommerceInherit for this fix might cause confusion. What do you think?
About removing isCustomInheritGlobalQueryImplementationEnabled, let's do it in a separated PR for better history.
Definitely, Also this doesn't fall into scope of this PR.
There was a problem hiding this comment.
Since this fix is not directly related to "custom inherit from query," using __woocommerceInherit for this fix might cause confusion. What do you think?
@imanish003 Because we're removing that "custom inherit feature", and the Products (Beta) block will be deprecated eventually, I don't think that's a big issue. But by the way, any name will work, I will try to find another one to clear any potential confusion.
Not sure why but it's still showing 9 products in Products beta block 🤔 Are you able to replicate this?
Let's pair on this.
There was a problem hiding this comment.
But by the way, any name will work, I will try to find another one to clear any potential confusion.
Thank you so much for your understanding. Since we're unsure who will be working on removing the "custom inherit query" feature, it's better to avoid any potential confusion if we can. 🙌🏻
|
@imanish003 I updated my PR to fix the issue with Products Beta block, can you please take another look at this? |
imanish003
approved these changes
Jul 26, 2023
|
Something to clarify:
cc @imanish003 |
imanish003
added
type: bug
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10228
In this PR, we're using two different approaches to fix the same problem:
Products (Beta)block, as we're extending the Query Loop block, we expose the__woocommerceInheritparameter to the REST query then modify query using therest_product_queryfilter to inject the correct loop items per page value to theposts_per_pageargument.Product Collectionblock, because we have total control over the codebase, we're following what Gutenberg does with the Query Loop block by setting the correct loop items per page directly in the edit component.Other Checks
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Because this PR updates the
Products (Beta), I checked the box forWooCommerce Coretoo.Performance Impact
N/a
Changelog