-
Notifications
You must be signed in to change notification settings - Fork 219
Product Search Results template: fix the preview when the Inherit Query option is enabled #7965
Product Search Results template: fix the preview when the Inherit Query option is enabled #7965
Conversation
The release ZIP for this PR is accessible via:
|
TypeScript Errors ReportFiles with errors: 432 🎉 🎉 This PR does not introduce new TS errors. |
Size Change: +109 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
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. |
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. |
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. |
f89afa8
to
654b9bc
Compare
6b7f546
to
da8089b
Compare
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. |
You can already review it. This PR includes changes from #7956. I suggest reviewing #7956 and, after that, reviewing this PR. |
Great work! 👏
I see @danieldudzic is already assigned to review that one, so I'll focus here, but heads up, some tests here are failing (both on the CI and locally). User facing test results:
✅ Confirmed that when the sale status filter is enabled, only products on sale are displayed
✅ Confirmed all products are displayed when the Inherit query from template option is enabled.
✅ Confirmed only on-sale products are visible. e2e test results:The tests on this branch are failing. When running the product-query test suite locally, it also failed, so probably worth investigating the causes. |
Thanks for the review!
The E2E tests fail in trunk too. I will take a look at them next week! |
07da73a
to
a8e49aa
Compare
a8e49aa
to
fc5ccce
Compare
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.
I tested the PR & it's working as expected. I left one minor suggestion but pre-approving PR because that's not a blocker 🚀
<Control | ||
{ ...props } | ||
queryObjectBeforeInheritEnabled={ | ||
queryObjectBeforeInheritEnabled | ||
} | ||
defaultWooQueryParams={ | ||
defaultWooQueryParams | ||
} | ||
key={ key } | ||
/> |
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.
AFAIU queryObjectBeforeInheritEnabled
& defaultWooQueryParams
will be used in only wooInherit
. If that's the case then I think instead of passing these as props, we should calculate it inside wooInherit
function.
As we are using hooks (ex. usePrevious
), I believe we will need to extract the wooInherit function as a component, something like this.
wooInherit: WooInheritToggleControl
IMO this would make the code easier to understand, but please feel free to ignore my suggestion if you disagree.
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.
Nice suggestion! I addressed it via 33fbac9.
I tested it, and it works. Please, could you recheck it?
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.
Hi @gigitux,
There is one issue. If I set keyword in filters as shown in screenshot below:
And then enable Inherit query from template
then, keyword effect is still there, i.e., I still see products that are filtered by the Keyword.
Is it expected? AFAIK filters shouldn't have any effect when Inherit query from template
is enabled. I might be wrong 🤷🏻♂️
That is a good point, but I can replicate this issue on the trunk. I think that it is a bug in the Query Loop block. Should we open another issue or write the issue on WordPress/gutenberg#29438? |
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.
@gigitux You are right. We can open a new issue on Gutenberg repo. Approving the PR 🚀
Although I think the issue you mentioned is a different one & not related to Inherit query from template toggle. You can also see my comment on the issue.
Thanks for the review!
Can you take care of this? |
Sure, I will take care of this 🤝 |
With this PR, when the user disabled the "Inherit Query option", the previous options are restored.
Fixes #7921
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility