Add lazy loading to images about netlify-plugin-cloudinary

[Vinitvh]

@colbyfayock No issues, you can assign @anirudhprabhakaran3 this one.

[Vinitvh]

Hi, I would like to work on this.

[colbyfayock]

hey sure! just a heads up, if you're participating in Hacktoberfest, i might recommend creating the PR after October 1st :)

let me know if you have any questions!

[Vinitvh]

Okay. Thank you.

[Vinitvh]

Hi @colbyfayock I have added loading=lazy attribute to img tag. I'm not sure how do I configure netlify.toml for setting it off. Need some help here.

[colbyfayock]

@Vinitvh here are some reasources:

• https://docs.netlify.com/integrations/build-plugins/create-plugins/#inputs

And he's an example of me already using it:

• https://github.com/colbyfayock/netlify-plugin-cloudinary/blob/main/netlify.toml#L13
• https://github.com/colbyfayock/netlify-plugin-cloudinary/blob/main/src/index.js#L33

You capture the value from the inputs argument passed into the build as seen in the 2nd example link.

Once there you can use it to control the change that you're making, where likely i would recommend doing something similar to this:

• https://github.com/colbyfayock/netlify-plugin-cloudinary/blob/main/src/index.js#L192

But I would create a separate function that does it's own processing on the HTML outside of the Cloudinary context

So something like:

  const { loadingStrategy = 'lazy' } = inputs;
  ...
  const { html, errors } = await updateHtmlImageLoading(sourceHtml, {
    loadingStrategy
  });

[Vinitvh]

@colbyfayock Thank you. Will go through these links and submit a PR.

[colbyfayock]

@Vinitvh sounds great. i also encourage you to take a moment to write test cases that check for both the default option and turning it off

might make sense to do that here: https://github.com/colbyfayock/netlify-plugin-cloudinary/blob/main/tests/lib/cloudinary.test.js#L87

[anirudhprabhakaran3]

Hi! Is it okay if I work on this issue? @Vinitvh are you still working on it? Thank you!

[colbyfayock]

if @Vinitvh is no longer interested, you can work on it, but they have the first opportunity

[colbyfayock]

sounds good, feel free to find another issue later if you have any interest!

[developer-diganta]

Hi! @anirudhprabhakaran3 ! Just wanted to know if you are still working on it? If not, will it be Ok if I work on it?

[colbyfayock]

since it's been 6 days since they last commented, I'm going to give them until Wednesday to respond

@Vinitvh please let use know if you plan on continuing with this ticket by Wednesday otherwise @developer-diganta gets next assignment

[Vinitvh]

Since @anirudhprabhakaran3 was assigned, I have stopped working on this. You can assign @developer-diganta, no problem from my side.

[colbyfayock]

Oops I got mixed up. Are you working on this @anirudhprabhakaran3?

[anirudhprabhakaran3]

Hi! Yeah, i was looking into the issue, but haven't been able to make headway... If i don't have progress by Wednesday, please assign it to someone else. Thank you!

[colbyfayock]

@developer-diganta are you still interested in this one?

[developer-diganta]

Yeah sure!

[colbyfayock]

assigned :)

[developer-diganta]

@colbyfayock Can you please help me out a bit? So I get whether the user wants lazy loading or not using const { loadingStrategy = 'lazy' } = inputs; and then inside updateHtmlImagesToCloudinary should I check the loadingStrategy and then set the attribute if required?

[colbyfayock]

yup exactly!

if you see inside of the code, we're using getAttribute to get the src https://github.com/colbyfayock/netlify-plugin-cloudinary/blob/main/src/lib/cloudinary.js#L189

we'd want to do something similar, where we would instead want to SET the attribute based on that option

async function updateHtmlImagesToCloudinary(html, options = {}) {
  const {
    assets,
    deliveryType,
    uploadPreset,
    folder,
    localDir,
    remoteHost,
    loadingStrategy
  } = options;

$img.setAttribute('loading', loadingStrategy);

along those lines

[colbyfayock]

also please write a test. there should be some nice examples inside of the tests file checking the HTML with the changes but lmk if you have questions about that as well

[developer-diganta]

@colbyfayock and inside the function, I'll have to check whether loadingStrategy was true and if so, I'll set the loading attribute right?

[colbyfayock]

well loadingStrategy in our example is set to lazy by default so you theoretically can pass that value right in to the image tag:

https://developer.mozilla.org/en-US/docs/Web/Performance/Lazy_loading#images_and_iframes

if someone is setting it as something else, they're explicitly saying they want something else

my thought process of using loadingStrategy instead of just loading (the attribute name) is to give opportunity to use it for other things in the future that aren't just using that tag

make sense?

so you set the attribute on the image with the value of the option

[developer-diganta]

@colbyfayock Can you please tell how to test this (I mean run the plugin and see)?

[colbyfayock]

once dependencies are installed you can run yarn test in the project root

[developer-diganta]

@colbyfayock Thanks! Could you please tell me how to resolve this error?

[colbyfayock]

@developer-diganta i'm not sure what changes you made that's making the URL change, this for instance would be a breaking change hence the test correctly failing

you need to look at the code changes you made to determine where the failure is and resolve it

[developer-diganta]

@colbyfayock Is there any env file that I have to add? I think it is not able to properly form the url because of that.

[colbyfayock]

just tried locally without an .env and it fails some tests, but the one that you showed seems to be a different issue

here is the .env:

CLOUDINARY_CLOUD_NAME="<Your>"
CLOUDINARY_API_KEY="<Your>"
CLOUDINARY_API_SECRET="<Your>"
NETLIFY_HOST="<Your>"

documentation for all but NETLIFY_HOST https://github.com/colbyfayock/netlify-plugin-cloudinary#environment-variables

[developer-diganta]

@colbyfayock since I am adding cloudinary_cloud_name as the one that I have created from cloudinary, should I also change the tests' expect urls cloud name with the my one? And also how can I get the NETLIFY_HOST?

[colbyfayock]

if im not mistaken the tests shoudl run with whatever keys are in the environment variables dynamically

  describe('getCloudinaryUrl', () => {

    test('should create a Cloudinary URL with delivery type of fetch from a local image', async () => {
      const { cloudinaryUrl } = await getCloudinaryUrl({
        deliveryType: 'fetch',
        path: '/images/stranger-things-dustin.jpeg',
        localDir: '/tests/images',
        remoteHost: 'https://cloudinary.netlify.app'
      });

      expect(cloudinaryUrl).toEqual(`https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/fetch/f_auto,q_auto/https://cloudinary.netlify.app/images/stranger-things-dustin.jpeg`);
    });

For instance - see how we're using a template literal to inject the environment variable in what's expected from the URL

ideally for any tests we can make them not dependent on specific variables or accounts

[developer-diganta]

@colbyfayock there's only one test failing now which is this one

I tried searching why is there a 5C to the url but to no avail. I do not think that the code change could have added this since it's setting a simple attribute. And this test fails even there was no change in code since I cloned a fresh copy and the test still fails. Can this be some path related issue(computer dependent)?

[colbyfayock]

5C is likely an encoded value

try checking out the main branch and running the tests. if they don't fail there, the issue is the code

definitely possible it's computer dependent though

if you're on windows, maybe the filepath is not working properly? thats something you can look into. haven't ran into this situation before with others

[developer-diganta]

I tried with the main branch (without my code addition) and it still failed. The error was the same.

[developer-diganta]

Shall I make PR so that you could give it a check?

[colbyfayock]

Yup that works. Plus the tests pass on the actions environment so we can see

[developer-diganta]

@colbyfayock I have made the PR

[colbyfayock]

hey @developer-diganta I can close this ticket now right?

[developer-diganta]

Yeah Sure!