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

Update 3rd-party compatibilities for multiple next-gen formats #836

Merged
merged 34 commits into from
Mar 20, 2024

Conversation

remyperona
Copy link
Contributor

Description

Update some 3rd-party compatibilities to support the updates in 2.2 for next-gen formats

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Checklists

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I did not introduce unecessary complexity.

Miraeld and others added 30 commits January 3, 2024 14:16
Co-authored-by: Rémy Perona <remperona@gmail.com>
@remyperona remyperona requested a review from a team March 4, 2024 21:47
@remyperona remyperona self-assigned this Mar 4, 2024
Base automatically changed from feature/avif to develop March 6, 2024 14:54
@piotrbak piotrbak added this to the 2.2.1 milestone Mar 7, 2024
@Mai-Saad
Copy link

@Tabrisrp @piotrbak can you please share the scenarios related to compatibility with amp and regenerate thumbnail plugins 🙏

@remyperona
Copy link
Contributor Author

There is no functional code changes here, it's only filter and variables names change. But the cases are:

  • for AMP, not display picture tag on AMP pages when the picture option is enabled
  • for regenerate thumbnails, auto-optimize and generate next-gen for images that are regenerated

@Mai-Saad
Copy link

Mai-Saad commented Mar 13, 2024

@Tabrisrp Thanks for the feedback.
Exploratory test notes
Amp
Notes already on 2.1.3

  • with Amp transitional mode, if we visited amp page while display webp using picture is enabled/disabled, only single image will be displayed (tested on page with image gallery of 3 images L, M, S while display on , only Large image '1st image in media list' displayed) => same with imagify deactivated
  • with standard setup of amp , visiting page?amp will redirect to page , however, images in source still in amp tag and not rewritten to image

Woocommerce
already on 2.1.3 (may need GH)

  • For variable product, we rewrite product image to webp but not the one in the jquery.zoom.min.js. However, if we clear variations, the image in zoom request will be next-gen => shouldn't we unify the process here, either always rewrite image in zoom request or don't rewrite it (I think it's related to the fact that zoom won't be as efficient as it should when webp is used when clear variation selection, the image in zoom is rewritten to webp and in this case zoom won't be much)
    Screenshot from 2024-03-15 13-28-55

  • we arenot serving next-gen for Variant images (served from optimized not webP)

  • if we uploaded new product image, processing in that page will keep loading although image is optimized

Regenerate thumbnail
Already on 2.1.3

  • if we manually deleted next-gen files for certain thmbnail , then we regenerated thumbnail, the webP won't be created for those thumbnails (at thumbnail, this is checked Skip regenerating existing correctly sized thumbnails (faster).)

Copy link

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

working as same as on trunk
Uploading testrail-report-586.pdf…

@remyperona remyperona removed this from the 2.2.1 milestone Mar 19, 2024
@Mai-Saad Mai-Saad added this pull request to the merge queue Mar 20, 2024
Merged via the queue into develop with commit a92dd2c Mar 20, 2024
7 of 8 checks passed
@Mai-Saad Mai-Saad deleted the fix/3rd-party-next-gen branch March 20, 2024 08:28
@piotrbak piotrbak added this to the 2.2.1 milestone Mar 21, 2024
@remyperona remyperona mentioned this pull request Mar 22, 2024
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.

7 participants