Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Configure Dominant Color and Fetchpriority modules for their standalone plugins #704

Merged
merged 10 commits into from
Apr 17, 2023

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Apr 6, 2023

Summary

Fixes #640.

Do not merge this pull request until the two plugin repositories for these two standalone plugins have been created and approved. Likely this will only be as part of the 2.3.0 milestone.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure [Focus] Images labels Apr 6, 2023
@felixarntz felixarntz added this to the 2.3.0 milestone Apr 6, 2023
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Found one issue that needs to be addressed before this is merged.

Also, since we're duplicating a lot of code between all the modules in their respective hooks.php and load.php files to handle things like avoiding conflicts with the Performance Lab plugin and outputting generator meta tags. I think there is an opportunity to consolidate these types of things to a shared file that got included in each plugin during the build process. There are tradeoffs to consider, for sure, but something we may want to consider.

Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>
@felixarntz
Copy link
Member Author

@joemcgill

Also, since we're duplicating a lot of code between all the modules in their respective hooks.php and load.php files to handle things like avoiding conflicts with the Performance Lab plugin and outputting generator meta tags. I think there is an opportunity to consolidate these types of things to a shared file that got included in each plugin during the build process. There are tradeoffs to consider, for sure, but something we may want to consider.

Agreed. Let's open an issue to think about that further. While coming up with centralized logic for this is probably trivial, we'll likely need to spend some time considering different approaches for how we handle this central logic that still needs to be somehow copied and bundled into all of the standalone plugins.

@felixarntz felixarntz requested a review from joemcgill April 6, 2023 20:41
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Sounds good to me. 👍🏻

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 @felixarntz! PR look good to me.

@felixarntz
Copy link
Member Author

@mukeshpanchal27 @10upsimon Since there was a merge conflict and since the two changes are so closely related, I've updated this PR to include the changes in the branch from #708 as well.

You can ignore those changes in here for now - let's complete and merge #708 first, and after that we can review the changes actually coming from this PR here. It should only be merged after the 2.2.0 release anyway.

@felixarntz felixarntz merged commit 338fa43 into trunk Apr 17, 2023
@felixarntz felixarntz deleted the add/other-images-standalone-plugins branch April 17, 2023 18:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up additional standalone plugins for Dominant Color and Fetchpriority
4 participants