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

Backport/55415 make duotone support compatible with enhanced pagination #5540

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Oct 20, 2023

Co-authored with @luisherranz

Trac ticket: https://core.trac.wordpress.org/ticket/59694

This PR backports #55415 that fixes an issue with duotone and enhanced pagination. All the context is inside Gutenberg PR.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@cbravobernal cbravobernal marked this pull request as draft October 20, 2023 11:55
@cbravobernal cbravobernal changed the base branch from 6.4 to trunk October 20, 2023 11:55
@cbravobernal cbravobernal marked this pull request as ready for review October 20, 2023 11:55
@cbravobernal
Copy link
Contributor Author

cbravobernal commented Oct 20, 2023

I'm not finding an easy way to test that in render_duotone_support() function with an empty $block_content, we are not returning early like we did before.

I tried checking that get_all_global_style_block_names() is called, but, as is a private static function, I couldn't make it work with getMockBuilder or ReflectionMethod

Any help would be kindly appreciated.

EDIT: Tests done 😄

@cbravobernal cbravobernal force-pushed the backport/55415-make-duotone-support-compatible-with-enhanced-pagination branch from 152b4ea to d264e88 Compare October 23, 2023 09:44
@cbravobernal
Copy link
Contributor Author

Hi there!

Pinging @tellthemachines for a review 😅
Thanks a lot!!

@cbravobernal
Copy link
Contributor Author

cbravobernal commented Oct 23, 2023

== Patch Report

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5540.diff

How to read this report:
🐞 <= Bug occurs.
✅ <= Behavior is ''expected''.
❌ <= Behavior is ''NOT expected''.

==== Environment

  • OS: macOS 14.0
  • Web Server: Nginx
  • macOS 14.0
  • PHP 8.2.11
  • 6.4-RC1-56475-src
  • Firefox 118.0.2

==== Steps to Reproduce

  1. With WordPress 6.4 installed
  2. Create the three posts with featured image, and another three without featured image.
  3. On the site editor, in home page, TT4, add pagination to the Query Loop, set it to inherit the template settings, activate "Enhanced Pagination feature)
  4. Add duotone to the feature image inside the Query Loop.
  5. Make sure the three posts shown in the home page don't have featured image. Use pagination.
  6. 🐞 Check that on page two, duotone filter is not applied for next posts.

==== Steps to Test Patch

  1. 🛠 Apply [https://patch-diff.githubusercontent.com/raw/Backport/55415 make duotone support compatible with enhanced pagination #5540.diff the patch].
  2. Refresh the home page.
  3. ✅ Check that the duotone filter is applied after navigating to the second page.
  4. ✅ Check that the navigation happened without a page reload.

==== Test Results

  • ✅ Issue is reproducible.
  • ✅ Issue resolved with patch.

==== Screenshots
Before patch:
Screenshot 2023-10-23 at 17 33 32

After patch:

Screenshot 2023-10-23 at 17 33 10

@hellofromtonya
Copy link
Contributor

For commit prep, I'll be pushing a few commits to this PR to address the failing PHP 8.3 test, review notes from @aaronjorbin, resetting the reflected property's visibility in the test, and a few other minor formatting things.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Ready commit

@hellofromtonya
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants