This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add: Interactivity Active Filters block #11996
Closed
Conversation
This file contains 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
Get the query params based on $query_id phpcs:ignore Word...Get the query params based on $query_id phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
woocommerce-blocks/src/BlockTypes/CollectionActiveFilters.php Lines 154 to 166 in 87f526f 🚀 This comment was generated by the automations bot based on a
|
dinhtungdu
added
skip-changelog
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/collection-filters/inner-blocks/active-filters/components/inspector.tsx
assets/js/blocks/collection-filters/inner-blocks/active-filters/index.tsx |
|
Size Change: +5.21 kB (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
samueljseay
approved these changes
Dec 1, 2023
| $filter['options'], | ||
| function( $acc, $option ) use ( $attributes ) { | ||
|
|
||
| $element_attributes = array_reduce( |
There was a problem hiding this comment.
This could be a useful utility function, like pass an assoc array and generate some element attributes. I could imagine it'd be useful for the increasing amount of server side rendering we're doing.
There was a problem hiding this comment.
I extracted the logic to its own method (private for now). If we see the real needs of using that method, we can later extract it.
| ); | ||
|
|
||
| $acc .= sprintf( | ||
| 'chips' === $attributes['displayStyle'] ? self::CHIP_ITEM_TEMPLATE : self::LIST_ITEM_TEMPLATE, |
There was a problem hiding this comment.
I think the approach you're taking to rendering here is not too easy to maintain personally. Its maybe personal bias, but we lose both syntactic HTML highlighting, proper HTML indentation, and also we have to go through the string templates and find/understand where the replacements are happening. not to mention there is also conditional choosing of template to consider here. I think what this ends up being with all that combined is not going to be nice for enhancing and maintaining. We should maintain something that feels more like a template. If you don't want to use obs_start etc, then I think we still need to do better than sprintf plain strings personally. 🤔
We can write something like trunk...add/interactivity-rating-filter#diff-ae77672452fc51e0d46de3599c96ed15f984ce5704e70a1799171d4084b6e80eR73-R86 that imho is just more readable and maintainable.
| return $acc; | ||
| }, | ||
| '' | ||
| ); |
There was a problem hiding this comment.
The array reduce furthers the code smell in my opinion. With closing PHP tags we can just insert loops in our html code to render list items and others. I think you're jumping too many hoops to avoid it and it feels imho worse as a result.
|
|
||
| if ( $formatted_min_price && $formatted_max_price ) { | ||
| /* translators: %1$s and %2$s are the formatted minimum and maximum prices respectively. */ | ||
| $title = sprintf( __( 'Between %1$s and %2$s', 'woo-gutenberg-products-block' ), $formatted_min_price, $formatted_max_price ); |
There was a problem hiding this comment.
It looks like a styling issue causes the spans to swallow up the whitespace between the word " and "
There was a problem hiding this comment.
I fixed this one by removing the HTML tag in the price, I think it's sufficient enough in that case.
|
@samueljseay I refactored the |
samueljseay
reviewed
Dec 4, 2023
| const url = new URL( window.location.href ); | ||
| const { searchParams } = url; | ||
|
|
||
| params.forEach( ( param ) => searchParams.delete( param ) ); |
There was a problem hiding this comment.
I noticed there is a bug here it will remove params such as post ID and will navigate away from a product for example. you'll just need to allow-list a list of all the product filters for now probably?
dinhtungdu
force-pushed
the
add/active-filters-interactivity-api
branch
from
5fb55c5
to
8238c10
Compare
|
@samueljseay fixed the issue with the clear all button in 8238c10, can you take another look at this PR? |
|
@dinhtungdu I see a bug where clearing the price filter breaks it (although I was able to see this sometimes clearing other filters) Step 1, set the filter: Step 2, clear it and the appearance of price filter breaks: |
dinhtungdu
force-pushed
the
add/active-filters-interactivity-api
branch
from
8238c10
to
860e286
Compare
|
@samueljseay I fixed the issue related to the wrapper context of collection filters block. The filter blocks should work both inside or outside of the Product Collection now. But I can't reproduce your issue with the price filter, can you give me a more detailed reproduce steps as well as any console/debug.log error? Clicking on the |
|
Closing in favor of woocommerce/woocommerce#42008 |
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.
What
This PR adds the new Active Filters Block powered by Interactivity API. It has the same features as the current one and works with new filter blocks.
Fixes woocommerce/woocommerce#42179
Why
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
Collection Filtersblock into aProduct Collectionblock.Collection Active Filtersand other filter blocks added along with theCollection Filtersblock.Clear Allbutton remove all filters.Screenshots or screencast
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog