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

Adding "imgClass" prop to the CldImage vue component #148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alxistn
Copy link

@alxistn alxistn commented Aug 27, 2021

Brief Summary of Changes

After submitting an issue to the repo (cf. #145) , I am submitting this pull request to add the possibility to pass a css class down from the <cld-image> component to the generated <img> tag.

What does this PR address?

Are tests included?

  • Yes
  • No

Reviewer, Please Note:

As of today there is no possible way to style the img tag using css class and the component <cld-image> because all css class added to the <cld-image> (eg. <cld-image class="my-css-class">) will apply to the <div> container not the <img> tag.

@@ -86,7 +94,7 @@ export default {
this.imageLoaded = true;
},
renderImageOnly(src, hasPlaceholder = false) {
const imgClass = `${IMAGE_CLASSES.DEFAULT} ${!this.imageLoaded ? IMAGE_CLASSES.LOADING : IMAGE_CLASSES.LOADED}`
const imgClass = `${IMAGE_CLASSES.DEFAULT} ${!this.imageLoaded ? IMAGE_CLASSES.LOADING : IMAGE_CLASSES.LOADED} ${this.imgClass}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that this adds an empty space at the end in case this.imgClass is empty, and thus our tests in Travis are failing (You can see the breaking tests on the pull request page).

Maybe we can trim extra spaces at the end to avoid this problem?

Copy link
Author

Choose a reason for hiding this comment

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

I've added a ternary operation that add the class if it is not empty, it seems to works fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, one last thing - can you please add a test that confirms that this feature works as expected?
I'd imagine a test that uses this new propr and confirms that the class appears on the right element in the DOM to be enough.

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.

2 participants