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
Add: Active Filters block powered by Interactivity API #42008
Conversation
Test Results SummaryCommit SHA: 43a05a2
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. |
Hi @samueljseay, 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: |
293140f
to
bf4044f
Compare
eadfd91
to
c94ab40
Compare
c94ab40
to
6980650
Compare
1ce377a
to
2c6175d
Compare
6c92ccb
to
d14e42d
Compare
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.
Tests well for me. Just some nitpicks mainly. 🚢
.../js/blocks/collection-filters/inner-blocks/active-filters/components/removable-list-item.tsx
Outdated
Show resolved
Hide resolved
.../js/blocks/collection-filters/inner-blocks/active-filters/components/removable-list-item.tsx
Outdated
Show resolved
Hide resolved
...ocommerce-blocks/assets/js/blocks/collection-filters/inner-blocks/active-filters/frontend.ts
Show resolved
Hide resolved
*/ | ||
import { BlockEditProps } from '@wordpress/blocks'; | ||
|
||
export type BlockAttributes = { |
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.
So I've been following this pattern too, mainly due to copy/paste, but do we really need separate file to declare types like this? i think it can live with the block?
nitpick really, it doesn't matter too much
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.
If we don't use attribute outside of the edit component, then we don't need this file. But usually, we need to use block attributes and/or edit props in other components, then a separate file makes more sense to me. Because it's something happy more often than not, I think following the 'convention' (even when the attribute is used only in one place) make it easier for every one working on the source.
1646a14
to
b74b6c7
Compare
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.
One nitpick suggestion from me 😄 pre-approving
E2E tests passed after one retry. Merging. |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #42179 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Collection Filters
block into aProduct Collection
block.Collection Active Filters
and other filter blocks added along with theCollection Filters
block.Clear All
button remove all filters.Note that the price filter nêd to be updated to use
context
instead ofstate
to make it work properly withClear All
button.Changelog entry
Significance
Type
Message
Comment
Add new Active Filters block powered by Interactivity API.