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

@uppy/thumbnail-generator: don't enlarge already small thumbnails #4406

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Apr 7, 2023

  • Thumbnail shouldn’t be enlarged / upscaled, only reduced. If img is already smaller than width/height, leave it as is. @nqst reported this in Single File Mode: fix layout and make optional #4374 (review)
  • We have image width/height when we resize for thumbnail, might as well set it as file.meta for Uppy users and us to use in the UI?

Before:
Screen Shot 2023-04-07 at 01 57 55

After:
Screen Shot 2023-04-07 at 01 56 55

@arturi arturi requested a review from mifi April 7, 2023 11:53
@@ -161,6 +161,9 @@ export default class ThumbnailGenerator extends UIPlugin {

return Promise.all([onload, orientationPromise])
.then(([image, orientation]) => {
const originalImageDimensions = { width: image.width, height: image.height }
this.uppy.setFileMeta(file.id, { dimensions: originalImageDimensions })
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this being stored somewhere else? is thumbnail-generator the right place to add this metadata? (it doesn't have anything to do with thumbnails I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean, stored somewhere else? Already, you mean? Not that I know of.

No, it’s not the right place, ideally we have a separate exif plugin that does this. But:

  1. It’s available “for free” here, we already read the original image to memory, so I thought why not store useful data (some people have asked for it).
  2. Alex asks the preview to behave differently, according to it’s size — if the image is smaller than container, like a tiny emoji, we have a white background for it, for instance. Ideally we need to know the image dimensions in CSS. So this would help here.

I also understand it doesn't feel “correct” to add this here, but since I was in this territory, decided to experiment and gather feedback.

Copy link
Contributor

@mifi mifi Apr 10, 2023

Choose a reason for hiding this comment

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

What do you mean, stored somewhere else? Already, you mean? Not that I know of.

I just thought that width and height is very common metadata for an image so I thought we are already setting it somewhere else in the code.

  • It’s available “for free” here, we already read the original image to memory, so I thought why not store useful data (some people have asked for it).

Yea, but then if someone needs the dimensions, but they don't want the thumbnail plugin, then they won't get it. and if we later add this to core, then there will be two different width/height metadata, which would be confusing.

2. Alex asks the preview to behave differently, according to it’s size — if the image is smaller than container, like a tiny emoji, we have a white background for it, for instance. Ideally we need to know the image dimensions in CSS. So this would help here.

Shouldn't it instead be stored inside plugin state then, because it's a thumbnail plugin specific state?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it instead be stored inside plugin state then, because it's a thumbnail plugin specific state?

👍

@arturi arturi self-assigned this Jun 6, 2023
@kvz kvz assigned Murderlon and unassigned arturi Sep 26, 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.

3 participants