Making filter block contextual on the front end #10919
Conversation
handle other product taxonomieshandle other product taxonomies
woocommerce-blocks/src/BlockTypes/CollectionFilters.php Lines 210 to 221 in e413a30 🚀 This comment was generated by the automations bot based on a
|
dinhtungdu
added
the
skip-changelog
|
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: +16 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
dinhtungdu
changed the title
dinhtungdu
added
type: enhancement
src/BlockTypes/CollectionFilters.php
Outdated
| $params_mapping = array( | ||
| 'exclude' => 'exclude', | ||
| 'offset' => 'offset', | ||
| 'order' => 'order', | ||
| 'orderBy' => 'orderby', | ||
| 'pages' => 'page', | ||
| 'parents' => 'parent', | ||
| 'perPage' => 'per_page', | ||
| 'search' => 'search', | ||
| 'woocommerceStockStatus' => 'stock_status', | ||
| 'woocommerceOnSale' => 'on_sale', | ||
| 'woocommerceHandPickedProducts' => 'include', | ||
| ); | ||
|
|
||
| foreach ( $query as $key => $value ) { | ||
| if ( isset( $params_mapping[ $key ] ) ) { | ||
| $params[ $params_mapping[ $key ] ] = $value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Mm, I'm not convinced about this… 🤔
I've got two suggestions:
- Either let's not put in the
params_mappingany param that doesn't require any mapping. The downside of this solution is that we are allowing arbitrary key/value pairs. Probably harmless and, actually, future-proof in a way, but I understand if you want to avoid it. - Second solution would be to have an
$accepted_paramsarray which would just bearray( 'exclude', 'offset', 'order', … )and then do:
foreach ( $query as $key => $value ) {
if ( in_array( $key, $accepted_params ) ) {
$params[ $key ] = $value;
}
if ( isset( $params_mapping[ $key ] ) ) {
$params[ $params_mapping[ $key ] ] = $value;
}
}I guess I just don't like strings mapping to themselves haha. I think it would be more explicit and clear to differentiate params that are accepted as is, and params that needs conversion, if you know what I mean.
There was a problem hiding this comment.
Let my try to convince you...
- The reason I create a list of param here is, when devs look at this method, they'll know exactly what is being passed to the API, they don't have to jump to other classes to look for this information. When we update the Product Collection query, yes we have to update it here, but I consider it as the trade-off for explicitness.
- The logic I'm using here is, I consider there are 2 types of param: ones don't need transform the value and ones need. Breaking the first type into two types increase the complexity of the logic IMO. In the example you shared above, there is an additional condition, there is also the need of another variable declaration for
$accepted_paramsas well.
I would rewrite the loop like this to be more explicit:
foreach ( $params_mapping as $key => $mapped_key ) {
if ( isset( $query[ $key ] ) ) {
$params[ $mapped_key ] = $query[ $key ];
}
}
There was a problem hiding this comment.
Update: I updated this PR to do so.
There was a problem hiding this comment.
The reason I create a list of param here is, when devs look at this method, they'll know exactly what is being passed to the API […]
For me, that's why I was suggesting it. Because at first glance it was confusing to see: I was expecting params_mapping to be params that require some mapping! :D Then I started to notice that some of them don't require any transformation, and that was the first surprise. The second surprise was when I noticed some of them don't require almost any transformation, but just a bit (e.g. parents → parent), and I had confused this latter type for the former type. So, I had two moments of confusion.
The logic I'm using here is, I consider there are 2 types of param: ones don't need transform the value and ones need.
For me there are three types:
- Params that are passed as is.
- Params that require a key mapping.
- Params that require a value mapping.
So, personally, I remain unconvinced 🙃 but Code Review is a place for me to bounce ideas and this is not a big deal for me. So if you feel strongly about your approach, I'm totally fine with it: it's still very good code.
There was a problem hiding this comment.
Let me try with three param types and see how the code goes.
There was a problem hiding this comment.
@sunyatasattva can you please take a look at dadcb89 (#10919)?
There was a problem hiding this comment.
This looks good to me, if you're happy with it. If you're not convinced, feel free to revert. The comments are a nice add!
There was a problem hiding this comment.
@sunyatasattva I'm happy with using three params types, but not with the implementation, it's too imperative. Then I ended up with https://github.com/woocommerce/woocommerce-blocks/pull/10919/files/dcfbfd9765e85494c992f5eeefe78c216fbd3010..96eb82cbaa56cb7b35eb4e2b80b7dc6e56f17892. Can you please take another look and let me know what do you think? It's not really declarative as I wanted, but I think it's better to move on and comeback for refactoring later.
src/BlockTypes/CollectionFilters.php
Outdated
| } | ||
|
|
||
| if ( ! empty( $query['taxQuery'] ) ) { | ||
| foreach ( $query['taxQuery'] as $taxonomy => $value ) { |
There was a problem hiding this comment.
Ok, so, if this was JavaScript, what I would do is: I would transform my mapping object $params_mapping such as it would be a Record< string, string | ( key: string ) => string > if you know what I mean, so that I could iterate the keys in the same loop above and do all transformations there.
Then, I would refactor these little transformations into their own functions as to put away implementation details from this method.
I don't know if this is nice in PHP, but just a suggestion.
There was a problem hiding this comment.
Can you share an example in JS on the idea you're talking about? I want to make sure we're on the same page. We're doing the similar thing for editor anyway so it won't be thrown away.
so that I could iterate the keys in the same loop above and do all transformations there.
Do you mean processing top level keys and nested keys inside one loop? Also process the transformation for both value can passed as it is and value need different transformation?
Then, I would refactor these little transformations into their own functions as to put away implementation details from this method.
Please keep in mind that the current method is already implementation details. I'm happy to split it down btw, as long as it doesn't make the function harder to read.
Again, I think it's easier for us if you can provide some code demonstrate your idea.
There was a problem hiding this comment.
Can you share an example in JS on the idea you're talking about? I want to make sure we're on the same page. We're doing the similar thing for editor anyway so it won't be thrown away.
Maybe something like this?
const PARAMS_MAP = {
parents: "parent",
perPage: "per_page",
taxQuery: ( key ) => {
const map = {
product_cat: "category",
product_tag: "tag",
};
return map[ key ] || `_unstable_tax_${ key }`;
}
}
return Object.entries( query ).reduce( ( acc, [ key, value ] ) => {
const mapper = PARAMS_MAP[ key ];
const newKey = typeof mapper === "function" ? mapper( key ) : mapper;
acc[ newKey ] = value;
return acc;
}, {} );Do you mean processing top level keys and nested keys inside one loop? Also process the transformation for both value can passed as it is and value need different transformation?
Sorry not sure what you mean with this?
Please keep in mind that the current method is already implementation details. I'm happy to split it down btw, as long as it doesn't make the function harder to read.
Yes, that's a good point. Maybe I'm too trigger-happy when it comes to split stuff. But I like the idea of having the key/value conversion responsibility to be separate. But maybe it's too overkill!
There was a problem hiding this comment.
Do you mean processing top level keys and nested keys inside one loop?
Sorry not sure what you mean with this?
In the code above, you process keys of both top level param like parents, perPage, and nested keys like product_cat and product_tag. If we have like 5 or 10 params like taxQuery, that probably makes more sense to me to map everything in one place, but here we're doing so for just taxQuery.
Also process the transformation for both value can passed as it is and value need different transformation?
I meant, in this case, we have to transform the value of nested keys of taxQuery, they're originally arrays, but we need comma separated strings to pass it to the Store API.
Btw, just a note if we reuse the snippet above, I tested it and it didn't return the desired shape.
Screenshot
src/BlockTypes/CollectionFilters.php
Outdated
| } elseif ( 'product_tag' === $taxonomy ) { | ||
| $key = 'tag'; | ||
| } else { | ||
| $key = '_unstable_tax_' . $taxonomy; |
There was a problem hiding this comment.
Haha I didn't know of _unstable_tax_.
dinhtungdu
force-pushed
the
contextual-filter
branch
from
7277515 to
dadcb89
Compare
|
Bumping the PR to the next release |
| private function get_inner_blocks_recursive( $inner_blocks, &$results = array() ) { | ||
| if ( is_a( $inner_blocks, 'WP_Block_List' ) ) { | ||
| foreach ( $inner_blocks as $inner_block ) { | ||
| $results[] = $inner_block->name; | ||
| $this->get_inner_blocks_recursive( | ||
| $inner_block->inner_blocks, | ||
| $results | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return $results; | ||
| } |
There was a problem hiding this comment.
This function can be refactored into this:
private function get_inner_blocks_recursive( $inner_blocks, &$results = array() ) {
if ( ! is_a( $inner_blocks, 'WP_Block_List' ) ) {
return $results;
}
return array_reduce(
iterator_to_array( $inner_blocks ),
function( $acc, $inner_block ) {
$acc[] = $inner_block->name;
return $this->get_inner_blocks_recursive(
$inner_block->inner_blocks,
$acc
);
},
$results
);
}
There was a problem hiding this comment.
Do you think this looks better? 🤔 I kinda like the first version with the loop to be honest.
There was a problem hiding this comment.
Without the short circuit, I do, but iterator_to_array() only accepts arrays from PHP 8.2, so we need that short circuit, which kinda decreases the readability.
We can remove the short circuit to make the logic flow less divergent, but I don't really like the inline condition.
private function get_inner_blocks_recursive( $inner_blocks, &$results = array() ) {
return array_reduce(
is_a( $inner_blocks, 'WP_Block_List' ) ? iterator_to_array( $inner_blocks ) : array(),
function( $acc, $inner_block ) {
$acc[] = $inner_block->name;
return $this->get_inner_blocks_recursive(
$inner_block->inner_blocks,
$acc
);
},
$results
);
}
What
ProductCollectionDataroute, we can make all filter blocks contextual in one place.ProductCollectionblock query context to Store API ProductCollectionData route params.Partial fixes #10985
Why
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
$colleciton_data_params_mappingproperty as followCollectionFilter.phpfile:Product Collectionblock.Collection Filtersblock to the block above.Collection Filtersblock.Screenshots or screencast
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog