Skip to content
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] Refresh product collections that don't support the interactivity API #44631

Merged
merged 5 commits into from Feb 16, 2024

Conversation

samueljseay
Copy link
Contributor

@samueljseay samueljseay commented Feb 15, 2024

Changes proposed in this Pull Request:

The new interactivity API filter blocks don't refresh the page when updating a filter (in most cases) because the new product collection can handle that.

But we need to still support classic template and the soon to be deprecated "Products (Beta)" block. In this PR we add a flag to client side data for those cases, we use that flag to determine if a full page refresh is needed. Bottom line: if at least one of these is present on a page we will do full page refresh when updating the filters. This will be easy to change later as we do all the navigation for interactivity filters from a single function.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. In site editor -> templates, edit the template block, add the new interactivity filters and the classic template block. To avoid confusion also remove "product collection" from shop page if it happens to be there already.
  2. Load the shop page and change some of the filters, for each a refresh of the page should happen and the respective product collection should be properly filtered.
  3. Create a page or template with the interactivity filters nested inside "Products (Beta)". There will be warning not to do this for each block, but thats ok.
  4. Preview your new template or page and same as 2, change some filters and see a page refresh and see the filters applied.
  5. You can also test the current case in a template or page add "Product Collection (Beta)" with new interactivity filters nested (and ensure no classic or product beta block is on the page). Preview the new content and apply the filters. the page should not refresh but filters should still be applied.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

[Experimental] Add a flag that will force page refresh when combining interactivity filters with classic template or products (beta) blocks.

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 15, 2024
@woocommercebot woocommercebot requested review from a team and imanish003 and removed request for a team February 15, 2024 06:20
Copy link
Contributor

github-actions bot commented Feb 15, 2024

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:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@samueljseay samueljseay requested review from dinhtungdu and removed request for imanish003 February 15, 2024 06:23
@samueljseay samueljseay reopened this Feb 15, 2024
Copy link
Contributor

github-actions bot commented Feb 15, 2024

Test Results Summary

Commit SHA: acd6a55

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests308001703257m 16s

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.

Copy link
Member

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is testing great for me. I only have some comments on the testing instructions:

Create a page or template with the interactivity filters nested inside "Products (Beta)". There will be warning not to do this for each block, but thats ok.

I think we should also mention here that even new filter blocks work inside Products (Beta), we don't have a plan to remove the warning when used with Products (Beta) block or update the copy (which mentions the Product Collection only) because the former is being deprecated soon.

Approving because this is working and testing as expected.

@samueljseay
Copy link
Contributor Author

This PR doesn't have the Experimental prefix, the QA team will pick it up for testing. We should state that this can't be tested without #44489.

@dinhtungdu yes true, 🤔 should we maybe just add [Experimental] prefix here? Then we could do general testing of this when we merge #44489 ?

I think we should also mention here that even new filter blocks work inside Products (Beta), we don't have a plan to remove the warning when used with Products (Beta) block or update the copy (which mentions the Product Collection only) because the former is being deprecated soon.

When you say mention, you just want to add this detail in the PR description? Maybe its not too important if we mark this experimental, but of course happy to add more explanation.

@dinhtungdu
Copy link
Member

should we maybe just add [Experimental] prefix here? Then we could do general testing of this when we merge #44489 ?

@samueljseay Yeah, sounds good to me.

When you say mention, you just want to add this detail in the PR description? Maybe its not too important if we mark this experimental, but of course happy to add more explanation.

Yes, I mean the description of the PR. But if we mark this one as experimental, then I agree that it's not necessary.

@samueljseay samueljseay changed the title Refresh product collections that don't support the interactivity API [Experimental] Refresh product collections that don't support the interactivity API Feb 16, 2024
@samueljseay samueljseay reopened this Feb 16, 2024
@samueljseay samueljseay merged commit f513ce7 into trunk Feb 16, 2024
69 of 70 checks passed
@samueljseay samueljseay deleted the dev/refresh-legacy-collection-blocks branch February 16, 2024 02:51
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 16, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 16, 2024
@Stojdza Stojdza added status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants