cloudinary / pkg-cloudinary-core

Distribution repository for the Cloudinary JavaScript library. Cloudinary is an end-to-end solution for all your image and video needs.
MIT License
54 stars 28 forks source link

Allow adding custom classes to img tags created by image/imageTag #20

Closed ohadschn closed 7 years ago

ohadschn commented 7 years ago

Consider:

cl.image("foo", {alt: "bar", class: "myclass" })

The resulting img tag will have its alt attribute set to "bar" as expected, but the class will be cld-responsive.

roeeba commented 7 years ago

The SDK's image method uses the properties given to the method to set the HTMLImageElement attributes.

HTML elements use the className member for getting and setting the value of the class attribute of the specified element. See additional info in https://developer.mozilla.org/en-US/docs/Web/API/Element/className.

The following code: cl.image("foo", {alt: "bar", className: "myclass"})

will result in this HTML element: <img alt="bar" class="myclass" src="https://res.cloudinary.com/demo/image/upload/foo" data-src-cache="https://res.cloudinary.com/demo/image/upload/foo">

ohadschn commented 7 years ago

Thank you, that is good to know.

You really should document it somewhere. Even the fact that cl.image supports custom arbitrary attributes is pretty well hidden in a single docs page (that I could find) saying this: "The Cloudinary Image Tag helper method allows you to not only specify any Cloudinary transformations parameters, but also to specify regular HTML image tag attributes (e.g., alt, title, width, height)." No explicit explanation or sample for doing this is provided. I think modifying one of the existing README.md samples to include some attribute (like alt) would be nice.

As for the matter at hand, the resulting HTML element you presented is problematic, because it lacks the ld-responsive class. Ideally, you would special-handle the className case to add it (that is append the requested classed, rather than let them override ld-responsive).

eitanp461 commented 7 years ago

@ohadschn thank you for your feedback. I will pass it on to our documentation team.

In order for the created image to have the cld-responsive class you need to add the responsive attribute to it like so: console.log('class', cl.image("foo", { responsive: true, alt: "bar", className: "myclass"}));

Which outputs: <img alt="bar" class="myclass cld-responsive" src="https://res.cloudinary.com/demo/image/upload/foo" data-src-cache="https://res.cloudinary.com/demo/image/upload/foo">

ohadschn commented 7 years ago

Oh, that makes sense, I forgot about responsive: true. Should I close the issue or would you like to keep it open until the documentation is updated? Like I said adding a small example to this repo's README.md might be good enough too IMHO...

roeeba commented 7 years ago

Hi @ohadschn. I've opened a ticket to our documentation team. I will close this ticket now, but please feel free to reopen it or open a new one if you have any further questions.