-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: add more Woo conditions to Block Conditions module #2281
Conversation
Plugin build for 698f507 is ready 🛎️!
|
Bundle Size Diff
|
E2E TestsPlaywright Test Status: Performance ResultsserverResponse: 238, firstPaint: 165.75, domContentLoaded: 1568.3, loaded: 1569, firstContentfulPaint: 3476.4, firstBlock: 6714, type: 13.14, minType: 10.79, maxType: 18.52, typeContainer: 8.53, minTypeContainer: 8.19, maxTypeContainer: 9.34, focus: 34.41, minFocus: 27.74, maxFocus: 43.95, inserterOpen: 23.99, minInserterOpen: 21.34, maxInserterOpen: 37, inserterSearch: 0.75, minInserterSearch: 0.62, maxInserterSearch: 0.93, inserterHover: 3.41, minInserterHover: 2.82, maxInserterHover: 4.63, listViewOpen: 149.76, minListViewOpen: 131.75, maxListViewOpen: 170.55 |
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.
Quick final note on the review as well related to commit history. I'll be doing a review of the commit history once the code changes are done. That said, I'll note for now that the chore
commits should be merged into the feature commit. You don't have to fix that now if you want. I can do a review of the commit history at the end so you can just do all that then.
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.
This is great @HardeepAsrani! Your refactor of the condition code was excellent! We're almost there.
Just to help show a bit what I'm thinking with the commit history, I would have made the refactor
commit just the changes to the conditions. The rest I would have left as another commit that we're going to merge back into the feat
commit.
Thanks @carlalexander for the review. I wanted to ask about how you feel is the best way to manage commits here. Personally, I try not to alter commits to protect their integrity even if, at times, it makes history look a bit ugly. But what would you suggest here? We squash all of them together or some other way? |
@HardeepAsrani left you a message in slack. Let's try to find a time to connect and I can go over that with you. 🙂 |
This PR adds: - Product Category - Product Tag - Product Attribute conditions to Block Conditions module Closes: https://github.com/Codeinwp/otter-internals/issues/198
This PR implements the things pointed out in the PR review, including better readability for block conditions, moving wp-content to vendor folder and more.
8c94603
to
698f507
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.
@HardeepAsrani I ended up doing the rebasing because there were merge conflicts to resolve. If you'd have preferred to do it, let me know.
I also left a video in Slack about the refactor
commit and what I would have liked to see in it. But otherwise, amazing work. Thank you! 🥳
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes https://github.com/Codeinwp/otter-internals/issues/198.
Summary
This PR adds:
conditions to Block Conditions module.
Screenshots
Test instructions
Checklist before the final review