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

Try: Add a quality query param to optionally set jpeg quality to a low or medium value #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewserong
Copy link
Member

@andrewserong andrewserong commented Aug 7, 2020

WIP: Note this has not been manually tested.

This a speculative attempt to add a low and medium quality option to mshots when saving out the JPEG image (I've selected 50 and 72 as values, but these could be changed).

The use case here is for if we need to display a larger number of screenshots on a particular page, we might wish to use lower quality images at a higher resolution, to optimise image loading. Or, use a small, low quality image in an initial page load before swapping out for a higher quality image. The purpose here is to give us the flexibility to be able have images with smaller file sizes.

Changes proposed

Add a quality query param to the PHP to support setting a med or low image quality on the requested JPEG image.

To do

  • Check that this does not affect caching / cache busting for any existing generated images
  • Adding the quality=med param should give you a unique filename with the quality in the name of the file, and this should be a unique cache key, too (I think I've done this correctly, but this should be verified)
  • Test that the proposed values are desirable
  • Test manually to make sure this actually works (I started exploring a local dev environment in Try: Attempt to run mshots in a local development environment via Docker #22, but haven't gotten that working yet)
  • Anything else?

@deBhal
Copy link
Contributor

deBhal commented Jan 21, 2021

I like it!

I think the code might be a bit simpler if we use the number for the internal parameter, so when I get a chance I'll make that change and do some testing.

@andrewserong
Copy link
Member Author

Sounds good, thanks for taking this one over @deBhal!

@andrewserong andrewserong removed their assignment Jan 21, 2021
@lsl lsl self-assigned this Jan 28, 2021
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.

3 participants