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

Use resize_to_cover instead of resize_to_fit for all images #618

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

robbevp
Copy link
Member

@robbevp robbevp commented Aug 10, 2024

Our current usage of resize_to_fit doesn't really fit the way we display images. We are guaranteed a the maximum of the biggest dimension, but this means we can't be sure of the size of the smallest dimension. We places these images in cards where assume that they can be shown at ± the dimensions of the resize. This can result in low quality results when the image has a high aspect ratio.

For example: I recently added an image that has a ratio of 3/1 , this is 500px x 167px after using resize_to_fit: [500, 500]. If this is displayed at 500px/500px, the image won't be shown at a decent quality. If we use resize_to_cover: [500, 500] the result will be 1500px x 500px, so it can be safely placed in a 500px/500px container. (This was sadly the only image provided as album artwork.)

Two downsides:

  • This will result in larger files for every image that is not a square (Though only slightly larger if the ratio is close to 1/1)
  • This will result in new images urls for ALL images on the server, which might cause a temporary extra load the server. We could mitigate this with some A/B paths, so we can roll out this change over a period of time. I don't think that extra code is worth it.
  • I've added tests relevant to my changes.

@robbevp robbevp added the enhancement New feature or request label Aug 10, 2024
@robbevp robbevp requested a review from chvp August 10, 2024 07:55
@robbevp robbevp self-assigned this Aug 10, 2024
@robbevp robbevp changed the title Use resize_to_cover instead of resize_to_fit Use resize_to_cover instead of resize_to_fit for all images Aug 10, 2024
@chvp chvp merged commit de73dea into main Aug 10, 2024
2 checks passed
@chvp chvp deleted the enhc/images-resize-to-cover branch August 10, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants