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

feat: use nuxt image module #26

Merged
merged 11 commits into from
Jan 26, 2021
Merged

feat: use nuxt image module #26

merged 11 commits into from
Jan 26, 2021

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Jan 21, 2021

Resolve #25 by using <nuxt-img> tag and loading="lazy" (internally using intersectionObserver for maximum browser compatibility but usage is consistent with native <img> tag)

This is initial step for integrating image module.

By providing width and height attributes component can also generate responsive urls (srcset) and also we have possibility to use more advanced <nuxt-picture> element for posters which automatically uses modern format (like webp), fallback, highres/responsive variant and blurry image for placeholder. For this we need to pass CDN resizing arguments which I would suggest doing in another PR.

@vercel
Copy link

vercel bot commented Jan 21, 2021

@pi0 is attempting to deploy a commit to the Nuxt Movies Team on Vercel.

To accomplish this, @pi0 needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

@pi0 pi0 mentioned this pull request Jan 21, 2021
@anton-karlovskiy anton-karlovskiy self-requested a review January 21, 2021 19:20
@anton-karlovskiy anton-karlovskiy self-assigned this Jan 21, 2021
@anton-karlovskiy
Copy link
Member

@pi0
Thank you very much.
Let me check it out.

@addyosmani
Copy link
Member

PR LGTM @pi0. Thanks for filing it. I am keen for us to better understand the gaps left for image loading once nuxt-image lands as I would anticipate most Vue/Nuxt users are going to go use it directly instead of tackling image optimizations themselves via the <img> tag.

@anton-karlovskiy
Copy link
Member

@addyosmani @pi0

FYI: I'm looking into this PR and gathering some discussing points regarding the best practices in loading images.
I will report on them shortly.

@vercel
Copy link

vercel bot commented Jan 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nuxt-movies/nuxt-movies/zlb072jwb
✅ Preview: https://nuxt-movies-git-feat-nuxt-img.nuxt-movies.vercel.app

@anton-karlovskiy
Copy link
Member

@pi0
Could you please teach me about these errors? Thank you.
error

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 25, 2021

@pi0

I think nuxt-image module lazy-load images by default according to https://image.nuxtjs.org/nuxt-image#lazy but it looks like we are using loading="lazy".
Could you please correct me if I'm wrong? Thank you.

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 25, 2021

@addyosmani

I found that the performance has improved to a great extent by using nuxt-module.
Do we want to create and log before and after LH results comparison?
cc @pi0

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 25, 2021

@addyosmani @pi0

The project originally applied aspect-ratio to images using CSS.
I think we should get rid of it and just use the aspect-ratio feature of nuxt-module?
aspect-ratio

What do you think?

@pi0
Copy link
Member Author

pi0 commented Jan 25, 2021

Hi @anton-karlovskiy thanks for update.

  • Regarding error, is it still happening? If yes what is URL that causes error i can locally check
  • loading="lazy" is handled by <nuxt-img> for wider browser compat
  • Docs was for 0.0.4. I'm publishing updated docs now
  • Using aspect ratio (or simply providing width and height is more than recommanded when possible)
  • With last release, we also support <nuxt-img responsive> which auto generates srcSet for us. I yet have to configure provider here so for tmdb image CDN

@anton-karlovskiy
Copy link
Member

@pi0

Thank you for the update

Regarding error, is it still happening? If yes what is URL that causes error i can locally check

Yes, it's still occurring. And whenever refreshing the site, errors are stacked in the server console. I guess this error occurs to every new image URL.

Docs was for 0.0.4. I'm publishing updated docs now

The version still appears to be 0.02.
version

Using aspect ratio (or simply providing width and height is more than recommended when possible)

Then I will get rid of the original CSS aspect-ratio and use the aspect-ratio feature of nuxt-module by setting width and height.
cc @addyosmani

With last release, we also support which auto generates srcSet for us. I yet have to configure provider here so for tmdb image CDN

It's what I wanted to ask because we have failed Properly size images Lighthouse audit.
If you could teach me how to configure the provider, I could try it myself.
I'd like the updated docs to be available soon in the first place so that I can set up responsive images.

@anton-karlovskiy
Copy link
Member

@pi0

we have possibility to use more advanced element for posters which automatically uses modern format (like webp)

As far as I know, TMDB does not provide WebP format images for now.

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 25, 2021

@pi0

I've tried with width and height set up.
aspect-ratio
But layout shifting still occurs.
layout-shifting

One thing I've noticed is according to the docs:
docs
But I cannot see the width and height attributes in the DOM tree although I set them in the <nuxt-img /> component.
attributes

Could you please teach me what's wrong? Thank you.

cc @addyosmani @danielroe

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 26, 2021

@pi0 @atinux @danielroe cc @addyosmani

The original project had the transition (animation) when lazy-loading images using CSS. As we got rid of the original v-lazyload custom plugin and its relevant CSS, now the project does not have any transition when lazy-loading images.
I think it might be important to the UX.

Should we use https://vuejs.org/v2/api/#transition for replicating this animation or does the nuxt-image have an option for it?

The original project: https://movies.jason.codes/person/90633
transition

The current project: https://nuxt-movies-d4lvpnplw.vercel.app/
no-transition

@anton-karlovskiy
Copy link
Member

Hi @pi0
Thank you for pushing the TMDB CDN provider.
May I ask if you have got a chance to review my comments?

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 26, 2021

Hi @pi0

Could you possibly share with me some demo projects using nuxt-image based on the providers like Cloudinary or IPX and responsive API?
Thank you.

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

Dear @anton-karlovskiy as mentioned before we are developing image module so that are no reference projects using it out there! (a simple demo is planned to publish during announcement)

Comment on lines +11 to +19
if (width) {
operations += 'w' + width;
}
if (height) {
operations += (operations ? '_and_' : '') + 'h' + height;
}
if (operations) {
operations += '_bestv2';
}
Copy link
Member

Choose a reason for hiding this comment

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

It will be broken because the predefined TMDB size prefix strings are not consistent with the implementation.

@anton-karlovskiy
Copy link
Member

@pi0

Thank you for letting me know about the demo projects.
No worries about the review comments. I'm working on them.

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

@anton-karlovskiy Please see my comment regarding CDN changing. It probably worth doing it instead of working around for directly using TMDB url (replacing current provider i added) since it doesn't meet resize features we need for responsive sizes....

@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 26, 2021

@pi0

Regarding:

@anton-karlovskiy Please see my comment regarding CDN changing. It probably worth doing it instead of working around for directly using TMDB url (replacing current provider i added) since it doesn't meet resize features we need for responsive sizes....

Do you mean the following comment?

Regarding tmdb images: I think finally we would need a better CDN layer or using IPX (nuxt@image built-in resizer) for deployments so we can resize to actual rendered element and also leverage that generates sizes and srcSet. We also have options of 3rd party (imgix, cloudinary) and vercel resizer (unfrotounaly currently undocumented and is designed Nextjs only but we are in contact with them to make it ga possible)... I've to think about cons/pros and will keep you updated :)

To be honest, I think I have not understood it correctly.
Could you please elaborate once again? Thank you very much.

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

Issue is, image optimizers like cloudinary or imgix, allow setting size parameters to any value while with image.tmdb.org we can only choose from a predefined set of sizes so even if close, we still download and use bigger image than the one is displayed: (also other things like we lose possibility of size optimization, progressive jpeg/webp, responsive sizes)

image

For a static generated website, we can use nuxt generate that downloads images and resizes them using Sharp but since it is a dynamic serverless deployment, we need an endpoint that does this on demand. There are several way we can do this so finally we always use original format from tmdb and pass it through that image provider that does stuff for us.

@anton-karlovskiy
Copy link
Member

@pi0

Thank you for your explanation.
I understood.
Let me try with Cloudinary then. cc @addyosmani

One thing I'm concerned about is if we use that approach, I'm wondering if the website will end up experiencing original (large and high quality) images when users land on the website for the very first time.

And as long as there are some services providing a set of predefined image sizes like TMDB, I'm just wondering if we would be able to adapt to such services by opening a way to just using the native srcset and sizes attributes.
Of course, it's not so ideal as using the image optimizers like Cloudinary but the fact that there would be a way to use native srcset and sizes with nuxt-image could be better just in my opinion.
What do you think @addyosmani?

Out of curiosity, does nuxt-image module consider the pixel density when resizing images? Re: https://web.dev/codelab-density-descriptors/

@anton-karlovskiy
Copy link
Member

@pi0 @addyosmani

Do you think the free pricing plan (https://cloudinary.com/pricing) is enough for our project?
cloudinary

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

One thing I'm concerned about is if we use that approach, I'm wondering if the website will end up experiencing original (large and high quality) images when users land on the website for the very first time.

Actually we pass original image to CDN to resize it (so end users never download original size)

@anton-karlovskiy Please be patient before using Cloudinary 😄

We are in parallel working on image module and solutions around it and I'm trying to keep you updated. Ideally it should be best if we don't ask users to use a 3rd party service and doing it with less possible configuration. For vercel, finally they have to provide us resizer endpoint (which is currently only available for nextjs) but in the meantime we have an opensource solution (IPX) that can be added as a serverless function.

And as long as there are some services providing a set of predefined image sizes like TMDB, I'm just wondering if we would be able to adapt to such services by opening a way to just using the native srcset and sizes attributes.
Of course, it's not so ideal as using the image optimizers like Cloudinary but the fact that there would be a way to use native srcset and sizes with nuxt-image could be better just in my opinion.

You can always use src-set attribute for nuxt-img instead of responsive attribute that auto generates it. But this is for sure not something we are going to advice as best practice when image module

Out of curiosity, does nuxt-image module consider the pixel density when resizing images?

Yep it also generates 2x variants for <nuxt-picture> and <nuxt-img responsive>

I would appreciate if you can wait until i finish PR for cdn part and merge then feedbacks more than welcome for image module improvements :) Our goal is not having performant website but advocate a way to make performant websites with nuxt.

…et by modifiders turn to undefined in the provider
@anton-karlovskiy
Copy link
Member

anton-karlovskiy commented Jan 26, 2021

@pi0

Thank you for explaining. :)

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

No worries @anton-karlovskiy <3 BTW feel free merging this PR as is so you can focus on working on other areas. I can open next one for CDN migration

@anton-karlovskiy
Copy link
Member

@pi0

Yes, then after an hour or so of work, I'm going to merge this PR.

@anton-karlovskiy
Copy link
Member

@pi0

The placeholder images for videos are broken. They should use another baseURL but TMDB_IMAGE_URL is attached in the provider.
video

Is there any way to use different base URLs per image?

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

@anton-karlovskiy Yes we do support multi-providers too. I guess using provider="static" should fix it. After using CDN, we can pass both youtube and tmdb images through it.

Comment on lines +10 to +19
let operations = '';
if (width) {
operations += 'w' + width;
}
if (height) {
operations += (operations ? '_and_' : '') + 'h' + height;
}
if (operations) {
operations += '_bestv2';
}
Copy link
Member

Choose a reason for hiding this comment

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

@pi0

This might not be always true.
Actually, episode images are broken as w400_bestv2 is not available. Probably og:images will be broken as w500_bestv2 is not available.
episodes
og-image

Copy link
Member

Choose a reason for hiding this comment

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

No worries, I can fix this.

@pi0 pi0 closed this Jan 26, 2021
@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

Sorry i have to close since have no enough time to track at the moment. Will reopen after supporting CDN so have no issues

@anton-karlovskiy
Copy link
Member

@pi0

Oh, I was going to merge this PR.
Should we not merge this PR?

@pi0
Copy link
Member Author

pi0 commented Jan 26, 2021

@anton-karlovskiy If it is okay that there are known issues (broken images, limited sizes) i highly advice to merge and avoid stacking stuff in same PR. Otherwise if you prefer a clean change with final ideal changes then we shall wait :D

@anton-karlovskiy
Copy link
Member

@pi0

OK, then I think I will merge this PR and work on another area.

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.

Defer offscreen images
3 participants