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

🌟 [EPIC] Responsive images #2255

Open
1 of 8 tasks
roschaefer opened this issue Nov 16, 2019 · 10 comments
Open
1 of 8 tasks

🌟 [EPIC] Responsive images #2255

roschaefer opened this issue Nov 16, 2019 · 10 comments

Comments

@roschaefer
Copy link
Contributor

roschaefer commented Nov 16, 2019

🌟 EPIC

Steps to implement:

Backend (uploading images)

Webapp (displaying images)


A user mentioned me on our network: https://human-connection.social/post/c0c726e7-bdef-45ac-8391-b82d17f125e8/bin-ich-eigentlich-die-einzige#commentId-7fc18102-25cc-452a-8729-7ea269131cc8

Mit dem Safari Browser (11.1.2) geht das Laden der Startseite extrem langsam, dazu sind zuerst einmal die Bilder bis auf einen 1cm-Streifen abgeschnitten, sie bauen sich langsam langsam Stück für Stück nach oben hin auf.

English:

With the Safari Browser (11.1.2) the loading of the start page is extremely slow, first of all the pictures are cut off to a 1cm strip, they build up slowly bit by bit upwards.

I had a look into our network traffic and indeed, our images are quite large. To reprodue:

  1. Go to our index page: https://human-connection.social/
  2. Right click >> Inspect >> Network
  3. Order requests by size (largest at the top)

You will see that some of the avatars are larger than 2MB(!). For example this here:

Screenshot - 2019-11-16T112509 896

The avatar is so small that it's barely visible. There is no justification to send more than 2MB over the wire.

User Problem

Especially for mobile browsers it is important to get smaller resolutions of images. You have a bandwith quota and it will exceed quickly. Furthermore, on a slow internet connection, the images will load really slowly, that's what the user is complaining about (see above ☝️).

Implementation

In our legacy alpha, we used https://github.com/thumbor/thumbor. I think it's still a reasonable choice. We would have to change our kubernetes configuration in a way that there is another thumbor service which has the persistent volume claim on the uploads folder.

Design & Layout

🤷‍♂️

Validation

You would request a responsive image and get different resolution: https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images

@ogerly
Copy link
Contributor

ogerly commented Nov 21, 2019

#2168

@mattwr18
Copy link
Member

this is what @roschaefer shared as a possible first step https://github.com/lovell/sharp

@mattwr18
Copy link
Member

We are migrating the Avatar and Card components so we have control to add a ResponsiveImage component, I've put in a PR #2801 following this blog:
https://markus.oberlehner.net/blog/lazy-loading-responsive-images-with-vue/

which leverages lozad for lazy loading as a starting point
https://github.com/ApoorvSaxena/lozad.js/

@alina-beck
Copy link
Collaborator

alina-beck commented Feb 4, 2020

The way I see it we have two approaches on the table right now – each with their advantages and disadvantages:

Store one image, resize it on demand (partly implemented by PR #2602)

  • more flexible since we’re not bound to a small set of sizes
  • much easier to change sizes in case of a layout change
  • bad for performance since calculations are done on the fly
  • needs to be combined with a caching solution to scale

Store different image sizes, request the required size (partly implemented by PR #2605)

  • very good performance, since images are resized during upload
  • takes up more storage space
  • limits us to the use of the set sizes
  • hard to change sizes later on (we’d need to run a script over all stored images)

On the frontend we could still use srcset with both of the above solutions – if we want to. (Here's a good article about working with the srcset attribute: https://medium.com/@elad/a-complete-guide-for-responsive-images-b13db359c6c7.)

I’m leaning towards the first one because I’m not very comfortable with deciding on any fixed image sizes at this point. But I know @Mogge has more experience in this regard, so I’d be happy to hear your suggestions! Long-term I think the second one would be more efficient, though… 🤷‍♀

@mattwr18 @roschaefer what do you think?

@alina-beck
Copy link
Collaborator

Meeting notes (Feb 11th, with @roschaefer, @mattwr18, @Mogge, @ogerly)

Two different topics in the backend:

Storing images

We want to remove the images from our cluster and put them in an object storage. We will probably store them on Digital Ocean's Spaces. It's cheaper than our current solution and allows the backend to scale.

Delivering images

We want to use a CDN (Content Delivery Network) to deliver images – it works like a cache for the different image sizes that are requested and makes requests faster.

Solution no1: The Cloudflare CDN has built-in resizing of images, so it would be an all-in-one solution – but this feature is only available for Enterprise Accounts, so it is expensive.

Solution no2: Spaces has a built-in CDN that doesn't include a function for resizing images on the fly, so we can only use it when we decide to store different image sizes.

Solution no3: We store the images on Spaces, resize the images on the fly and configure our own CDN.

Frontend question:

Why use srcset instead of query parameters for sizes?

  • Plus: Because then we leave the decision which file size to request up to the browser and don't have to bother with screen resolutions etc (or so we hope)
  • Downside: Nobody on the team has experience with it yet
  • We'll try to use it and figure it out as we go!

Decided next steps:

  • Add GraphQL Image type, refactor the frontend to use it
  • Upload existing images to Spaces
  • Make sure future images are stored in Spaces
  • Test performance of resizing images on the fly
  • Test the effect of different images sizes on the file size, to be able to decide on fixed sizes

Related:

  • Write a script to delete images that are no longer used --> OR: upload only actually used ones to Spaces
  • Stop transforming uploaded images to jpg and compressing them in the frontend (done by cropper)
  • Compress uploaded images or limit the upload file size

What we want in the end:

  • Image upload with a file size limit (probably 5 MB)
  • Don't compress or transform images (maybe normalize them, though?)
  • Store original to Spaces
  • Resize images (either during the upload or on the fly, depending on the decision we make)
  • Send the smallest possible image file to the frontend for the requested size

Open questions:

  • How will we deal with vector graphics?
  • (How) do we want to normalize images?
  • Final decision (after testing, see ☝️): resize on the fly or store different sizes?

@mattwr18
Copy link
Member

mattwr18 commented Feb 11, 2020

really awesome @alina-beck !!! thank you soo much for taking such good concise notes!! It's very much appreciated.
I have a question and some feedback from having a look at the current TeaserImage uploads.

When writing cypress tests for Delete Teaser Image PR by @ogerly #2585 , I was running into the issue that we are using a canvas from cropperjs to convert the file to a blob, which does the compression. This is coming back as null in Cypress. I have tried several workarounds and didn't get anything to work.

I remember that we just had this discussion and decided.
a) we do not compress png or svg
b) @roschaefer said that compressing jpg doesn't have that big of an effect.

I have just added a check to see if an image is a png or svg and if so, not compressing, but if it's a jpg, compressing. We talked about not compressing jpg since it likely wouldn't make much of a difference, or maybe cause we want to save the original (?). I have just done some tests, and the images without compressing are about 3x larger, which is why we added it in the first place. see issue #1933 .

Question: Does this change anything for anyone? Or do we still want to say as long as it's less than 5mb, save the original?

@alina-beck
Copy link
Collaborator

I'd say leave the image compression for jpg in, for now – to make sure our file sizes don't blow up. Then, when we have implemented a solution for resizing images, remove the compression from the frontend as a last step. Does that sound ok?

@mattwr18
Copy link
Member

sounds good to me! anyone object?

@stale
Copy link

stale bot commented Apr 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 11, 2020
@stale stale bot closed this as completed May 11, 2020
Roadmap automation moved this from PRIOLIST (ALWAYS TO FINALISE) to Done May 11, 2020
@Mogge
Copy link
Member

Mogge commented May 13, 2020

not stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment