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

Fix funny compression (ABA-827) #1157

Merged
merged 1 commit into from
May 12, 2024
Merged

Fix funny compression (ABA-827) #1157

merged 1 commit into from
May 12, 2024

Conversation

PeterJFB
Copy link
Contributor

For some reason, image size was determined by the crop viewport 🙃

This means cropping a large image on a small screen would create a small image (and in theory vice versa).

This will likely resolve (ABA-827), as inserting an image is no longer determined by viewport size, but original image size.

This also means uploading a large image makes it large (assuming AWS has no compression, i has no checked).

Before:

Screenshot from 2024-04-14 18-36-44

After:
Screenshot from 2024-04-14 18-37-51

Copy link

linear bot commented Apr 14, 2024

@ivarnakken
Copy link
Member

I haven't tested, but this looks promising. What I don't understand is why this suddenly started happening?

Also, I believe there's compression on gallery images as well, but the image uploader for that is outside of lego-editor, which makes me suspect there might be something else causing this?

I've checked AWS, and there should be no compression on their side. Should probably investigate that further though

@eikhr
Copy link
Member

eikhr commented Apr 15, 2024

What I don't understand is why this suddenly started happening?

I believe image size has always been related to the size of the crop viewport, but the insane compression is definitely someting new.

@eikhr
Copy link
Member

eikhr commented Apr 15, 2024

Will this cause overflow issues if someone uploads very large images? Maybe we should have some maximum size, or maybe rather limit it in backend or just when displaying the images in the editor.

@PeterJFB
Copy link
Contributor Author

I haven't tested, but this looks promising. What I don't understand is why this suddenly started happening?

I assume the final size used to be determined by the Image and canvas-object created when inserting the image. This means any css-changes to the page or browser-update to object defaults could affect the image quality 🙃

Also, I believe there's compression on gallery images as well, but the image uploader for that is outside of lego-editor, which makes me suspect there might be something else causing this?

Might be, I've only looked at the images which have had this issue (org-kart), and the fix looks promising.

I've checked AWS, and there should be no compression on their side. Should probably investigate that further though

Agreed. 🙌

Will this cause overflow issues if someone uploads very large images? Maybe we should have some maximum size, or maybe rather limit it in backend or just when displaying the images in the editor.

One way to find out 😉. Should be easy to see if this has to be fixed in staging

@ivarnakken
Copy link
Member

Also, I believe there's compression on gallery images as well, but the image uploader for that is outside of lego-editor, which makes me suspect there might be something else causing this?

Might be, I've only looked at the images which have had this issue (org-kart), and the fix looks promising.

Have you looked at this?

Will this cause overflow issues if someone uploads very large images? Maybe we should have some maximum size, or maybe rather limit it in backend or just when displaying the images in the editor.

One way to find out 😉. Should be easy to see if this has to be fixed in staging

And this

@PeterJFB
Copy link
Contributor Author

Also, I believe there's compression on gallery images as well, but the image uploader for that is outside of lego-editor, which makes me suspect there might be something else causing this?

Might be, I've only looked at the images which have had this issue (org-kart), and the fix looks promising.

Have you looked at this?

Will this cause overflow issues if someone uploads very large images? Maybe we should have some maximum size, or maybe rather limit it in backend or just when displaying the images in the editor.

One way to find out 😉. Should be easy to see if this has to be fixed in staging

And this

Merge first, check later 😎, approve pls

@ivarnakken
Copy link
Member

Merge first, check later 😎, approve pls

what?

Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

Looks good. Definitely needs some more testing, but I think doing it in staging is ok. I think any image size limits we need to implement should be done on backend anyways.

@PeterJFB PeterJFB merged commit db589e0 into master May 12, 2024
3 checks passed
@PeterJFB PeterJFB deleted the funny-compression branch May 12, 2024 09:23
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