Skip to content

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

Merged
mitogh merged 10 commits intotrunkfrom
feature/174-full-image-size-in-content
Mar 10, 2022
Merged

Replace the full size image in the_content with additional MIME type if available#195
mitogh merged 10 commits intotrunkfrom
feature/174-full-image-size-in-content

Conversation

@mitogh
Copy link
Member

@mitogh mitogh commented Feb 28, 2022 •
edited
Loading

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.

@mitogh
Merge branch 'feature/149-update-the-content' into feature/174-full-i…
50e835b 
…mage-size-in-content
@mitogh
Replace full size image in the content.
3866bec 
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 requested a review from felixarntz February 28, 2022 20:00
@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
adamsilverstein
adamsilverstein approved these changes Mar 8, 2022
View reviewed changes
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!

felixarntz
felixarntz reviewed Mar 8, 2022
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.

@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.

@mitogh mitogh requested a review from felixarntz March 9, 2022 00:58
felixarntz
felixarntz approved these changes Mar 9, 2022
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.

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

felixarntz commented Mar 9, 2022

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

Reviewers

@adamsilverstein adamsilverstein adamsilverstein approved these changes

@felixarntz felixarntz felixarntz approved these changes

@getsource getsource Awaiting requested review from getsource

Assignees

@mitogh mitogh

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

Milestone

1.0.0-beta.2

Development

Successfully merging this pull request may close these issues.

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

3 participants

@mitogh @felixarntz @adamsilverstein