[Product Query] Add support for the Filter By Price Block #7162
Conversation
|
The release ZIP for this PR is accessible via: |
8839d75 to
7818f5d
Compare
|
Size Change: 0 B Total Size: 903 kB ℹ️ View Unchanged
|
7818f5d to
db1030e
Compare
Product Query - Add support for the Filter By Price Block
db1030e to
6e93dd0
Compare
There was a problem hiding this comment.
That's really cool. 🤩 Really exciting to see that this works in parallel to the On sale filter, the potential is huge! Good job! 👏
I found a strange behavior. When no products match the price filter, all products are shown (ie, setting the price filter to 33-34):
I also left a couple of inline comments, but they are quite minor.
Oh, and can you set the sprint of this PR? 🙏
src/BlockTypes/ProductQuery.php
Outdated
| @@ -79,24 +82,39 @@ public function get_query_by_attributes( $query ) { | |||
| 'post_status' => 'publish', | |||
| 'posts_per_page' => $query['posts_per_page'], | |||
| 'orderby' => $query['orderby'], | |||
| 'orderby' => $query['orderby'], | |||
There was a problem hiding this comment.
This line is duplicated, is that a typo?
src/BlockTypes/ProductQuery.php
Outdated
| return array( | ||
| 'on_sale' => ( ! isset( $variation_props['attributes']['query']['onSale'] ) || true !== $variation_props['attributes']['query']['onSale'] ) ? array() : $this->get_on_sale_products_query(), | ||
| ); |
There was a problem hiding this comment.
I know it's not introduced in this PR, but I wonder if we could extract this check into its own variable to make the code easier to read, something like:
| return array( | |
| 'on_sale' => ( ! isset( $variation_props['attributes']['query']['onSale'] ) || true !== $variation_props['attributes']['query']['onSale'] ) ? array() : $this->get_on_sale_products_query(), | |
| ); | |
| $on_sale = ! isset( $variation_props['attributes']['query']['onSale'] ) || true !== $variation_props['attributes']['query']['onSale']; | |
| return array( | |
| 'on_sale' => $on_sale ? array() : $this->get_on_sale_products_query(), | |
| ); |
30445ec to
81c0f5c
Compare
|
Thanks for the review and for testing the PR. I addressed all your feedback! b243528 The relation in the query was wrong. It was I noticed this strange behavior. above WC Core With the same range, but with the ExplorationIt seems that it isn't easy to find the What do you think? |
81c0f5c to
b243528
Compare
| $max_price_query, | ||
| $min_price_query, |
There was a problem hiding this comment.
Should we check that $max_price_query and $min_price_query are not an empty array, here? Otherwise we end up with queries in this form:
[meta_query] => Array
(
[relation] => AND
[0] => Array
(
)
[1] => Array
(
)
)
(I have no idea if that's an issue, though, so feel free to ignore this comment if you think that's ok)
There was a problem hiding this comment.
src/BlockTypes/ProductQuery.php
Outdated
| */ | ||
| private function get_on_sale_products_query() { | ||
| return array( | ||
| 'post_in' => wc_get_product_ids_on_sale(), |
There was a problem hiding this comment.
Should this be post__in (with two underscores), instead?
src/BlockTypes/ProductQuery.php
Outdated
| function( $acc, $query ) { | ||
| // Ignoring the warning of not using meta queries. | ||
| // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query | ||
| $acc['meta_query'] = isset( $query['meta_query'] ) ? array_merge( $acc['meta_query'], array( $query['meta_query'] ) ) : $acc['meta_query']; | ||
| return $acc; | ||
| }, |
There was a problem hiding this comment.
If we go with the post__in approach, this array_reduce() is ignoring that property, right? Because we are only taking the meta_query value from $queries_attributes and $queries_filters, but not post__in. I might be missing something, though. 🤔
|
With the last refactor I broke:
Thanks for your review and comments! Now, it should be fine! 🤞 |
There was a problem hiding this comment.
Thanks for updating this PR, @gigitux! This is working fine. I left one minor comment about how we merge the post__in attribute, instead of accumulating all ids I think we should only accumulate the duplicated ones. But besides this, LGTM so pre-approving.
src/BlockTypes/ProductQuery.php
Outdated
| if ( isset( $query['post__in'] ) ) { | ||
| $acc['post__in'] = isset( $acc['post__in'] ) ? array_merge( $acc['post__in'], $query['post__in'] ) : $query['post__in']; | ||
| } |
There was a problem hiding this comment.
If I'm understanding this correctly, I think this is working now because we only set the post__in attribute with the on sale filter. But if at some point there were two filters using post__in, instead of concatenating the ids we should only include the intersection of $acc['post__in'] and $query['post__in'], in other words, ids which are in both arrays so it behaves as an AND instead of an OR.
There was a problem hiding this comment.
Good point! I fixed this!
…ocks into add/price_filter_support
d4d1f2a to
0d7a39a
Compare
|
Thanks for the review! 🥳 |
…e#7162) * Product Query: Fix pagination issue * Product Query - Add support for the Filter By Price Block woocommerce#6790 Product Query - Add support for the Filter By Price Block * fix query relation * fix on sale query * fix bugged pagination and on-sale filter after refactor * fix reload page when the Filter By Price block is used with All Products block * use array_intersect instead of array_merge
This PR adds support for the Filter By Price block.
The code is written to be easily extendible and adding easily the support to other filters and other variations (frontend side/editor side).
#6790
Screenshots
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog