This repository was archived by the owner on Feb 23, 2024. It is now read-only.
[Product Query] Add support for the Filter By Price Block#7162
Merged
[Product Query] Add support for the Filter By Price Block#7162
Conversation
Contributor
|
The release ZIP for this PR is accessible via: |
8839d75 to
7818f5d
Compare
Contributor
|
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
5 tasks
Aljullu
reviewed
Sep 16, 2022
Contributor
Aljullu
left a comment
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? 🙏
| 'post_status' => 'publish', | ||
| 'posts_per_page' => $query['posts_per_page'], | ||
| 'orderby' => $query['orderby'], | ||
| 'orderby' => $query['orderby'], |
Contributor
There was a problem hiding this comment.
This line is duplicated, is that a typo?
Comment on lines
+166
to
+168
| return array( | ||
| 'on_sale' => ( ! isset( $variation_props['attributes']['query']['onSale'] ) || true !== $variation_props['attributes']['query']['onSale'] ) ? array() : $this->get_on_sale_products_query(), | ||
| ); |
Contributor
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:
Suggested change
| 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
Contributor
Author
|
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
4 tasks
Aljullu
reviewed
Sep 21, 2022
Comment on lines
+183
to
+184
| $max_price_query, | ||
| $min_price_query, |
Contributor
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)
Contributor
Author
There was a problem hiding this comment.
| */ | ||
| private function get_on_sale_products_query() { | ||
| return array( | ||
| 'post_in' => wc_get_product_ids_on_sale(), |
Contributor
There was a problem hiding this comment.
Should this be post__in (with two underscores), instead?
Comment on lines
+99
to
+104
| 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; | ||
| }, |
Contributor
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. 🤔
Contributor
Author
|
With the last refactor I broke:
Thanks for your review and comments! Now, it should be fine! 🤞 |
Aljullu
approved these changes
Sep 23, 2022
Contributor
Aljullu
left a comment
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.
Comment on lines
+101
to
+103
| if ( isset( $query['post__in'] ) ) { | ||
| $acc['post__in'] = isset( $acc['post__in'] ) ? array_merge( $acc['post__in'], $query['post__in'] ) : $query['post__in']; | ||
| } |
Contributor
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.
Contributor
Author
There was a problem hiding this comment.
Good point! I fixed this!
…ocks into add/price_filter_support
d4d1f2a to
0d7a39a
Compare
Contributor
Author
|
Thanks for the review! 🥳 |
Merged
98 tasks
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 12, 2022
…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 file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
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