-
Notifications
You must be signed in to change notification settings - Fork 219
Register patterns under patterns
folder and add filters pattern
#6861
Conversation
The release ZIP for this PR is accessible via:
|
Size Change: +392 B (0%) Total Size: 872 kB
ℹ️ View Unchanged
|
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
patterns
folder and add filters pattern
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.
I have tested this and functionally it works as expected, and as you described! I am provisionally approving this PR but I have added some comments which I think could be useful to add to this change.
I might be wrong but I think you need to line separate the linked issues to ensure this PR closes them automatically when this is merged.
|
@tjcafferkey Thanks for the feedback. I pushed some changes, could you have another quick 👀 ? Thanks! |
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 adding those changes, they look good to me! My regex is not amazing so descriptive variables/comments go a long way in terms of readability so thanks for that.
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.
@albarin Just a minor comment, are you planning to have more classes in src/Patterns
?
If not, I think we can move it from src/Patterns/Patterns.php
to src/Patterns.php
, or even more explicit: src/BlockPatterns.php
.
Not for now, I think, I like your |
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.
@albarin Great updates! I left some more comments related to the coding style, please take a look.
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.
Good feedback @dinhtungdu !
@albarin I've review and retested and it looks good to me!
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.
LGTM! Thanks so much for the updates! I left an optional comment but pre-approving 🚀.
This PR introduces the new
Patterns
class that is used to register patterns and pattern categories.The patterns are loaded from the
/patterns
folder. It also introduces a new pattern under theWooCommerce
category with all four existing filter blocks (by attribute, price, stock, and active filters).The code to load the patterns from the folder comes from WordPress.
Fixes #6838
Fixes #6839
Fixes #6840
Testing
User Facing Testing
All Products
block.WooCommerce Filters
patterns
folder of the plugin. The file has to start with a format like:WooCommerce Visibility
Changelog