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
Product Collection: disable "Sync with current query" option for 2nd+ block in product archive #44577
Product Collection: disable "Sync with current query" option for 2nd+ block in product archive #44577
Conversation
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: ccaf4fc
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.
Great work so far! I left two minor suggestions regarding the title of the tests. Here are the testing results:
⛔ Scenario 1: After adding the last Product Collection (Beta) block, the "Sync with current query" is not disabled:
CleanShot.2024-02-15.at.12.55.35.mp4
✅ Scenario 2: After adding the last Product Collection (Beta) block, the "Sync with current query" setting of the last added block is enabled.
CleanShot.2024-02-15.at.13.05.00.mp4
✅ Scenario 3: When I add the Product Collection (Beta) block to a new post, the ""Sync with current query" option is not available. If I switch to the Code Editor view, the "inherit" is set to false
.
CleanShot.2024-02-15.at.13.06.51.mp4
...woocommerce-blocks/tests/e2e/tests/product-collection/product-collection.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
...woocommerce-blocks/tests/e2e/tests/product-collection/product-collection.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexandre Lara <allexandrelara@gmail.com>
@thealexandrelara , thanks for a review! I addressed minor comments 👍
TBH I can't reproduce it at all. Is there any chance the project didn't rebuild on your side? I'm asking because scenarios 2 and 3 are only regression scenarios, meaning they would pass on |
…current-query-for-2nd+-block
I tried rebuilding or creating a JN site from this branch, but still getting the same results, unless I'm doing something wrong while testing or there is some requirement that I'm missing. I'm going to send the JN site credentials to you in private :) |
…roduct Collection blocks are nested
@thealexandrelara , I tested adding blocks on a higher level, while you tested with nested blocks. I misread one selector documentation I didn't notice it wouldn't work for nested blocks. I revisited implementation. May I ask you for yet another 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.
Great work! Thank you for addressing all the raised points. I just tested it and everything is working as expected. 🚀
Submission Review Guidelines:
Changes proposed in this Pull Request:
There's a "Sync with current query" option available on Product Collection block (Product Catalog collection). Till now the option was enabled by default on product archives. This PR introduces the logic that the option is enabled ONLY IF there's no other Product Collection block that already syncs with query.
In other words, the rule of thumb is:
Closes #44086
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Scenario 1
Scenario 2
Scenario 3
"inherit"
and make sure it'sfalse
Changelog entry
Significance
Type
Message
Comment