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

Replace the full size image in the_content with additional MIME type if available #195

Merged
merged 10 commits into from
Mar 10, 2022

Conversation

mitogh
Copy link
Member

@mitogh mitogh commented Feb 28, 2022

Summary

Fixes #174

This PR requires:

Relevant technical choices

This PR updates the changes introduced by #152 and #194 in order to replace the full size image if present in the new format, (in this case WebP) making sure all images served to the client are in the same format when a new image is uploaded into WordPress.

Checklist

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

If the full size image is present as part of the metadata
information, make sure the image is replaced in the content
with the new reference to the expected image.

This fixes #174
@mitogh mitogh self-assigned this Feb 28, 2022
@mitogh mitogh added [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Feb 28, 2022
@mitogh mitogh added this to the 1.0.0-beta.1 milestone Feb 28, 2022
@mitogh mitogh added the [Type] Enhancement A suggestion for improvement of an existing feature label Feb 28, 2022
@mitogh mitogh changed the title Feature/174 full image size in content Replace the_content with the full size image in new mime types if available Feb 28, 2022
@mitogh mitogh modified the milestones: 1.0.0-beta.1, 1.0.0-beta.2 Feb 28, 2022
@mitogh mitogh changed the base branch from trunk to feature/174-full-image-size February 28, 2022 21:18
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Excellent!

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.

@mitogh The new code overall looks solid, I found a few small issues that mostly relate to the already existing code, but we had missed them before.

modules/images/webp-uploads/load.php Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Show resolved Hide resolved
@mitogh mitogh requested a review from felixarntz March 9, 2022 00:58
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.

LGTM!

Let's wait with merging this though until #194 is ready, since technically the implementation here depends on how we implement #194.

@felixarntz
Copy link
Member

Ah nevermind the above, I see you've created this against your branch for #194. I think for the future it would be better to create both PRs based on trunk, since if we merge this now, it would be harder to review #194 as it would include the changes from here which were already approved.

So I'd still say it's better to wait until #194 has been approved before we merge this.

@mitogh
Copy link
Member Author

mitogh commented Mar 9, 2022

Thanks, mostly I've created this based on #194 just to have smaller diffs I agree this should is currently being blocked by #194 but having everything based on trunk will create merge conflicts after approval which usually triggers another review, so when having this approved and PR against #194 allows for simpler flow, however, I do see the problem specifically around dependencies between each PR.

Base automatically changed from feature/174-full-image-size to trunk March 10, 2022 01:18
@mitogh mitogh merged commit d70c915 into trunk Mar 10, 2022
@mitogh mitogh deleted the feature/174-full-image-size-in-content branch March 10, 2022 01:19
@felixarntz felixarntz changed the title Replace the_content with the full size image in new mime types if available Replace the full size image in the_content with additional MIME type if available Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When generating WebPs, ensure full size is generated(-scaled for large images)
3 participants