-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Experimental] Use variations to provide all the product filters wrapped by CollectionFilters block #43216
Conversation
…on Filters block.
Hi @dinhtungdu, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: 442e98a
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this PR! This is working as expected according to the testing instructions. I don't have any comment codewise but about one on the behavior:
In the block editor the filter blocks by themselves should not show in the block inserter
I don't think we should do this, because:
- The filter blocks are only available inside
Collection Filters
block, so they don't add more noise to the global inserter. - With this PR, when adding inner blocks to
Collection Filters
, for example Price Filter, we're adding anotherCollection Filters
block andPrice Filter
blocks to the originalCollection Filters
block.
I think the behavior should be:
Collection Filters
block shouldn't be available inside otherCollection Filters
block.- The new variations added in this PR also shouldn't be available inside
Collection Filters
block. - The inserter visibility of new filter blocks should be restored as they are before, only available inside
Collection Filters
block. - In the other words, we should disallow the following blocks inside
Collection Filters
, so when inside that block, the only result, price filter for example, should be the new price filter block:- Filter Wrapper variations (we will disable legacy blocks soon so this may not necessary).
- Collection Filters block and its variations.
Sadly, Gutenberg hasn't provided any API to disallow specific inner blocks right now, so we have to either:
- use a fix list of allowed blocks, which includes new filter blocks and blocks for layout purpose.
- use a dynamic list of allow blocks (defining the list at run time).
…branch (#43207) Set WOOCOMMERCE_BLOCKS_PHASE to experimental when building PR live branch.
3cfea8e
to
d3e7b7b
Compare
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/block.json
Show resolved
Hide resolved
@dinhtungdu I took another go at this based on your feedback. Hurts the head a little when dealing with variants and such but let me try explain the behavior implemented. If you could test and see if what I've implemented here matches your expectations :)
I think that about sums it up. I expect we can fix up any block naming as part of #43167 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great update! Having the Collection Filters inserts all filter blocks by default is a good UX 💯 .
In my test, I can still insert variation inside another variation. Here is the testing video taken on https://shy-kingfisher.jurassic.ninja/
Screen.Recording.2024-01-03.at.18.12.09.mov
Checking the code, I can see you're splitting into two cases. I don't think we need those two cases because in every case, only the collection filter blocks can be added in inside Collection Filters
, not variations of Collection Filters
block.
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/block.json
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/edit.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/edit.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/index.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Tung Du <dinhtungdu@gmail.com>
Co-authored-by: Tung Du <dinhtungdu@gmail.com>
thanks @dinhtungdu I tested your changes and you're right I was overthinking the block filtering (I also learned that variations are not individual block types, neat!) I've applied your suggestions and ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! This is LGTM now.
plugins/woocommerce-blocks/assets/js/blocks/collection-filters/edit.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Tung Du <dinhtungdu@gmail.com>
Changes proposed in this Pull Request:
To support discoverability and simplify adding filters this PR registers each of the new interactivity filters as a variation of the collection-filters block and inside the block we choose a template for the respective filter based on an attribute passed in the variant.
Closes #42160
How to test the changes in this Pull Request:
Changelog entry
Significance
Type
Message
[Experimental] Use variations of collection filter to provide new interactivity based filter blocks.
Comment