search

cloudinary / cloudinary-react

React components that utilize Cloudinary functionality
MIT License
502 stars 219 forks source link

Does the Placeholder component still work? #232

Closed fcisio closed 2 years ago

fcisio commented 3 years ago

Bug report for Cloudinary React SDK

Before proceeding, please update to latest version and test if the issue persists

Describe the bug in a sentence or two.

I'm testing the package on the latest version, and I can't make the placeholder work. Nothing gets added to the DOM, it's the same as without.

Describe the desired/expected behavior.

A placeholder should appear in the DOM.

Link to reproduction of the issue on codepen/jsfiddle/etc.

None yet.

Issue Type (Can be multiple)

[ ] Build - Can’t install or import the SDK [ ] Babel - Babel errors or cross browser issues [x] UI/Performance - Display or performance issues [x] Behaviour - Functions aren’t working as expected (Such as generate URL) [x] Documentation - Inconsistency between the docs and behaviour [ ] Incorrect Types - For typescript users who are having problems with our d.ts files [ ] Other (Specify)

Steps to reproduce

I think a good plan would be for the package Storybook to be updated and add stories with use cases of Placeholder.

roeeba commented 3 years ago

Hi @fcisio, Would you mind sharing a sample of your code showing where the error occurs? Also, are you getting any error messages in the browser's console?

fcisio commented 3 years ago

Here's a code sandbox

I think I figured out my issue, basically, the placeholder only appears before the main image is loaded. After that, it is removed from the DOM, which is why I didn't see it.

Is it the intended behavior?

roeeba commented 3 years ago

@fcisio That's the intended behavior. Why would you like it to remain in the DOM?

fcisio commented 3 years ago

If it works it works, but I feel like it would make more sense to leave it in the DOM. That's what most of the other similar package I've encountered are doing.

It would probably prevent confusion.

I also know about @cloudinary/react which is the successor of this package, so this might not be a super important issue.

roeeba commented 3 years ago

Thanks for the feedback @fcisio. Can you please share why do you think it makes more sense to leave the placeholder in the DOM?

fcisio commented 3 years ago

Well, it would probably be a little lighter in JS since there wouldn't be a cleanup function. It would also prevent confusion with new users of the package. What you see in the DOM is what you get.

I think overall it would allow for a better understanding of what happens under the hood.

roeeba commented 3 years ago

Thanks, @fcisio. I'll forward it to the team to consider. Appreciate the feedback.