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.

Sign up for GitHub

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

Jump to bottom

Lazily load Duotone settings only when needed. #4831

Closed
wants to merge 22 commits into from
Closed

Lazily load Duotone settings only when needed. #4831

wants to merge 22 commits into from

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Jul 11, 2023 •
edited

This PR does a number of things.

  • Fixes the original issue by deferring loading of logic until there is a block that needs the data ( one with duo tone ). This remove logic from wp_loaded, that use resulting in the issue on install.
  • Lazily loading data on when needed, so when there is a block that support duo tone.
  • Improved readability by using the tblock_has_support util.
  • Check for block with null as block name ( can happen with empty spaces ).
  • Improved performance of render_duotone_support, by reuse type already passed in filter.
  • Fix tests that by passing all params to filter.

When testing this, it is important to test with pages / posts with and without blocks that support duo tone.

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

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.

felixarntz
felixarntz reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@spacedmonkey Nice work, lazy-loading the duotone configuration only when needed makes a lot of sense to me. Code looks solid to me and quite straightforward, simply replacing the variable access with calling the private method to lazy-load 👍

Can you share before & after profiling results to see how that changes overall load time?

ajlende
ajlende approved these changes Jul 11, 2023
View reviewed changes
Copy link

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable way to cache these two pieces rather than adding the action.

@spacedmonkey spacedmonkey self-assigned this Jul 12, 2023
@spacedmonkey spacedmonkey marked this pull request as ready for review July 12, 2023 11:37
mukeshpanchal27
mukeshpanchal27 reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @spacedmonkey, Left nit-pick

src/wp-includes/class-wp-duotone.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-duotone.php Outdated Show resolved Hide resolved
costdev
costdev requested changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the PR @spacedmonkey!

I tested this on 6.3-beta4 (patched with this PR) on Twenty-Twenty Three (classic themes continue to not load duotone), with Gutenberg active and inactive, using the following blocks:

Supports duotone

  • Site Logo
  • Post Featured Image
  • Image
  • Gallery
  • Cover

Doesn't support duotone

  • Media & Text
  • Video
  • Video with poster image

I also added global duotone filters for the first group above. These were correctly used when duotone filters weren't applied in the post editor, and were correctly ignored when duotone filters were applied in the post editor.

Note: When the Gutenberg plugin is active, duotone filters seem to get stronger. This seems to happen with 6.3-beta4 though, so I don't think this PR has anything to do with it. Not sure what's going on there (I'm on mobile so not checking release notes and code at the moment).

I've also included some docblock feedback in this review.

src/wp-includes/class-wp-duotone.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-duotone.php Outdated Show resolved Hide resolved
felixarntz
felixarntz reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment •
edited

Choose a reason for hiding this comment

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

@spacedmonkey Other than the one feedback from https://github.com/WordPress/wordpress-develop/pull/4831/files#r1261443459, this looks good to me code-wise.

While this looks like quite some refactoring, most line changes are just indented code, so I think that's fine. The ideas of changing the get_selector() method signature and usage to expect a WP_Block_Type instance and to use block_has_support() where applicable make sense to me and are fairly straightforward enhancements.

However, there seems to be a bug caused by this based on #4831 (review)? From that perspective, I am slightly worried about the refactoring in this PR. While I'm onboard with the PR in its current state, we may want to be more cautious and go with the much simpler change based on the changes from yesterday, before you refactored further.

@ajlende
Copy link

ajlende commented Jul 12, 2023

However, there seems to be a bug caused by this based on #4831 (review)? From that perspective, I am slightly worried about the refactoring in this PR. While I'm onboard with the PR in its current state, we may want to be more cautious and go with the much simpler change based on the changes from yesterday, before you refactored further.

I've tested it and it is fixed by f10a53f.

ajlende
ajlende reviewed Jul 12, 2023
View reviewed changes
Copy link

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

This is testing well for me.

With the addition of this suggestion https://github.com/WordPress/wordpress-develop/pull/4831/files#r1261443459, it gets a 👍 from me.

ajlende
ajlende approved these changes Jul 12, 2023
View reviewed changes
Copy link

@ajlende ajlende left a comment •
edited

Choose a reason for hiding this comment

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

This looks good to me. 👍

Thanks for working on this @spacedmonkey :)

tellthemachines
tellthemachines approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @spacedmonkey! This is testing well for me, both on its own and together with #4839. I can confirm duotone filters are working as expected both in the editor and on the front end, with block themes and (after applying #4839) with classic themes too.

@ajlende ajlende mentioned this pull request Jul 13, 2023
Closed
@swissspidy
Copy link
Member

Can we rebase this now that 4839 has been merged?

costdev
costdev approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the updates @spacedmonkey! As @swissspidy mentioned, a rebase would be good! 🙂

mukeshpanchal27
mukeshpanchal27 approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Great work @spacedmonkey

@spacedmonkey @swissspidy
Apply suggestions from code review
31233bd 
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
@spacedmonkey
Copy link
Member Author

Committed https://core.trac.wordpress.org/changeset/56226 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@tellthemachines tellthemachines tellthemachines approved these changes

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 approved these changes

@costdev costdev costdev approved these changes

@westonruter westonruter Awaiting requested review from westonruter

@youknowriad youknowriad Awaiting requested review from youknowriad

@joemcgill joemcgill Awaiting requested review from joemcgill

@audrasjb audrasjb Awaiting requested review from audrasjb

@SergeyBiryukov SergeyBiryukov Awaiting requested review from SergeyBiryukov

@felixarntz felixarntz Awaiting requested review from felixarntz

@ajlende ajlende Awaiting requested review from ajlende

Assignees

@spacedmonkey spacedmonkey

Labels
None yet
Projects
WordPress 6.3.x Editor Tasks
Status: ✅ Done
Milestone
No milestone
7 participants
@spacedmonkey @ajlende @swissspidy @felixarntz @tellthemachines @mukeshpanchal27 @costdev