This repository was archived by the owner on Feb 23, 2024. It is now read-only.
[Product Query] Add support for the Filter Products By Attributes block #7186
Conversation
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
|
The release ZIP for this PR is accessible via: |
|
Size Change: 0 B Total Size: 992 kB ℹ️ View Unchanged
|
8fe6700 to
166bd49
Compare
5 tasks
166bd49 to
a9fa429
Compare
Product Query - Add support for the Filter By Attributes block
a9fa429 to
926f62c
Compare
…erce/woocommerce-blocks into add/price_filter_support
…erce/woocommerce-blocks into add/attribute_filter_support
…commerce/woocommerce-blocks into add/attribute_filter_support
c771df8 to
0be510e
Compare
sunyatasattva
suggested changes
Sep 22, 2022
src/BlockTypes/ProductQuery.php
Outdated
| add_filter( 'query_vars', array( $this, 'set_query_vars' ) ); | ||
| // Set this so that our product filters can detect if it's a PHP template. | ||
| $this->asset_data_registry->add( 'has_filterable_products', true, true ); | ||
| $this->asset_data_registry->add( 'is_rendering_php_template', true, true ); |
There was a problem hiding this comment.
Sorry, I don't exactly know how this works, but should we always set this to true even when not rendering PHP templates? Or should we wrap this into a condition of some sort?
There was a problem hiding this comment.
I fixed this in the previous PR #7162
src/BlockTypes/ProductQuery.php
Outdated
| 'tax_query' => array(), | ||
| ); | ||
|
|
||
| $queries_attributes = $this->get_queries_by_attributes( $variation_props ); |
There was a problem hiding this comment.
This function seems to return a list of queries, right? So perhaps let's call this variable something different? Like $queries_by_attributes is the obvious one. Or just $custom_queries.
src/BlockTypes/ProductQuery.php
Outdated
| ); | ||
|
|
||
| $queries_attributes = $this->get_queries_by_attributes( $variation_props ); | ||
| $queries_filters = $this->get_queries_by_applied_filters(); |
There was a problem hiding this comment.
Same idea as above. Maybe let's call this $filters_queries if you wanna keep it similar to this. But AFAIU this returns the queries from the filters, and not filters themselves, right? So it'd be a bit more semantically appropriate, I feel.
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
110
to
120
| if ( isset( $query['post__in'] ) ) { | ||
| $acc['post__in'] = isset( $acc['post__in'] ) ? array_merge( $acc['post__in'], $query['post__in'] ) : $query['post__in']; | ||
| } | ||
| // 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']; | ||
| // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query | ||
| $acc['tax_query'] = isset( $query['tax_query'] ) ? array_merge( $acc['tax_query'], array( $query['tax_query'] ) ) : $acc['tax_query']; | ||
| return $acc; | ||
| }, | ||
| $common_query_values |
There was a problem hiding this comment.
I'd suggest we add post__in in $common_query_values as an empty array. In this way we can remove all the ternary operators in this function, I think, making it a bit easier to read. In the end, it's a super simple function, but it looks complicated honestly.
src/BlockTypes/AttributeFilter.php
Outdated
Comment on lines
14
to
15
| const FILTER_QUERY_VAR = 'filter_'; | ||
| const QUERY_TYPE_QUERY_VAR = 'query_type_'; |
There was a problem hiding this comment.
Do you think these need to be part of the AttributeFilter class? In the end, they only get used by our class, so maybe it makes more sense to move them there? Not sure though. What do you think?
I'd also rename them to something like FILTER_QUERY_VAR_PREFIX.
There was a problem hiding this comment.
I did the same thing for the `PriceFilter.php≠. These constants indicate the query arguments that are added to the URL when the filters are applied. For me, it makes sense, keeping them scoped with the filters.
src/BlockTypes/ProductQuery.php
Outdated
| * @return array | ||
| */ | ||
| private function get_filter_by_attributes_query_vars() { | ||
|
|
There was a problem hiding this comment.
Are empty lines at the beginning of the functions a thing we do or was this a typo?
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
169
to
176
| function( $acc, $attribute ) { | ||
| $acc[ $attribute->attribute_name ] = array( | ||
| 'filter' => AttributeFilter::FILTER_QUERY_VAR . $attribute->attribute_name, | ||
| 'query_type' => AttributeFilter::QUERY_TYPE_QUERY_VAR . $attribute->attribute_name, | ||
| ); | ||
| return $acc; | ||
| }, | ||
| array() |
There was a problem hiding this comment.
I'd probably refactor this function as it's own thing. Not so much because it will be reused, but to make things a bit clearer. Like generate_attribute_filters or something like that?
There was a problem hiding this comment.
Mmm, I don't see any advantages to creating a dedicated function. This function is pretty easy to understand and also the class is already huge.
src/BlockTypes/ProductQuery.php
Outdated
| $attribute_name_value = explode( ',', $attribute_name_value ); | ||
|
|
||
| $acc[] = array( | ||
| 'taxonomy' => str_replace( AttributeFilter::FILTER_QUERY_VAR, 'pa_', $attribute_name ), |
There was a problem hiding this comment.
What does pa_ stand for?
There was a problem hiding this comment.
Good question... the pa is the prefix for custom attributes:
We do a similar thing here
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
259
to
260
| $attribute_name_value = get_query_var( $attribute_name ); | ||
| $attribute_query_type_value = get_query_var( $attribute_query_type ); |
There was a problem hiding this comment.
I'd put just $attribute_value and $attribute_query.
src/BlockTypes/ProductQuery.php
Outdated
| return $acc; | ||
| } | ||
|
|
||
| $attribute_name_value = explode( ',', $attribute_name_value ); |
There was a problem hiding this comment.
Could you add some comments as to why we explode it here?
…ocks into add/attribute_filter_support
14 tasks
|
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 - otherwise it will automatically be closed after 10 days. |
Script Dependencies ReportThe
This comment was automatically generated by the |
sunyatasattva
approved these changes
Oct 17, 2022
There was a problem hiding this comment.
It works like a charm! I mean… it's exactly the experience we had expected (and I hope we don't decide to go to production with this version), but it works as specced so far.
Thanks for addressing my previous comments. I've left a few more, but they are minor. I'm pre-approving.
Comment on lines
+144
to
+150
| $attributes_filter_query_args = array_reduce( | ||
| array_values( $this->get_filter_by_attributes_query_vars() ), | ||
| function( $acc, $array ) { | ||
| return array_merge( array_values( $array ), $acc ); | ||
| }, | ||
| array() | ||
| ); |
There was a problem hiding this comment.
It looks great now, thanks! It helps a lot actually to follow what's the expected shape… until we get TypePHP 😬
| return $acc; | ||
| } | ||
|
|
||
| // It is necessary explode the value because $attribute_value can be a string with multiple values (e.g. "red,blue"). |
There was a problem hiding this comment.
Thank you for this comment!
src/BlockTypes/ProductQuery.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Intersect arrays when both are not empty, otherwise merge them. |
There was a problem hiding this comment.
Just a little grammar fix
Suggested change
| * Intersect arrays when both are not empty, otherwise merge them. | |
| * Intersect arrays when neither of them are empty, otherwise merge them. |
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
331
to
332
| * @param array $array1 Array. | ||
| * @param array $array2 Array. |
There was a problem hiding this comment.
You know from the other PR how I feel about these kind of names, but I don't want to be too obnoxious. Again, if we want to keep two args, I'd go for $a and $b, but my suggestion would be to mimic the signature of both array_merge and array_intersect and accept any number of arguments.
There was a problem hiding this comment.
I updated the function. Before merging the PR, I prefer that you take a last check :)
…ocks into add/attribute_filter_support
…commerce/woocommerce-blocks into add/attribute_filter_support
sunyatasattva
approved these changes
Oct 26, 2022
98 tasks
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 12, 2022
…ck (woocommerce#7186) * 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 * Product Query - Add support for the Filter By Attributes block woocommerce#6790 Product Query - Add support for the Filter By Attributes block * fix bugged pagination and on-sale filter after refactor * address feedback * address feedback
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.
This PR adds support for the Filter By Attributes block. This PR is blocked by #7162.
#6790
Screenshots
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog