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

Component with effect has a wrapping span with custom styles predefined #33

Closed
elamperti opened this issue Mar 31, 2019 · 13 comments · Fixed by #70
Closed

Component with effect has a wrapping span with custom styles predefined #33

elamperti opened this issue Mar 31, 2019 · 13 comments · Fixed by #70
Labels
enhancement New feature or request

Comments

@elamperti
Copy link

Bug description
When a has an effect property defined (e.g. "opacity"), the image is wrapped with a span (where all the corresponding classes are applied (e.g. lazy-load-image-loaded)). Said span comes with some styles already defined in the component, as seen in method getWrappedLazyLoadImage() of LazyLoadImage.jsx. This raises problems when the wrapping element is expected to have its default behavior. In my case, I needed it to stay displayed as a block (as an would be), but instead it becomes an inline-block. In my case it breaks my view (see screenshot below)

To Reproduce
Create a with an effect and inspect it.

<LazyLoadImage src="test.jpg" alt="Test" effect="opacity" />

Expected behavior
The wrapping element shouldn't change how the component is rendered.

Perhaps the style for the wrapper should be defined in the CSS of each effect instead.

Screenshots
Selection_107

Technical details:

  • Package version: 1.3.2
  • Server Side Rendering? No
  • Device: any
  • Operating System: doesn't apply
  • Browser: happens with both Firefox and Chrome

Maybe it is related -or can be circumvented- if #31 is implemented

@elamperti elamperti changed the title Wrapping span in component with effect comes with style property Component with effect has a wrapping span with custom styles predefined Mar 31, 2019
@Aljullu
Copy link
Owner

Aljullu commented Apr 20, 2019

Perhaps the style for the wrapper should be defined in the CSS of each effect instead.

This would be a good solution, the only issue I see is that the wrapper element is also used when placeholderSrc is defined, even if there is no effect. So we would need to create a new CSS file and ideally only import it if the wrapper element is displayed.

Thanks for filling this issue @elamperti, will mark it as enhancement in case I have time in the future or somebody is willing to pick it up!

@Aljullu Aljullu added the enhancement New feature or request label Apr 20, 2019
@asimoesmcartor
Copy link

@Aljullu I am also running into a similar issue caused by the pre-defined styles in the LazyLoadImage.jsx file on line 77.

Is there any way that you could remove the "display:inline-block" style in that span?

Or could you give more context on why you added this style?

@Aljullu
Copy link
Owner

Aljullu commented May 25, 2019

@asimoesmcartor the display: inline-block is required to make the placeholder visible. The placeholder is a <span> with a background image but no content, so it needs to be a block to have height and width. I used inline-block instead of block to replicate better the default behavior of images, which are inline according to browser default styles.

I'm considering to add a placeholderProps or wrapperProps prop so the style and other attributes can be overwritten. I guess that would be good enough to solve your issues here.

In the short term, you can set:

.lazy-load-image-background {
  display: block !important;
}

in your CSS and that would overwrite the default display value set by the library.

@asimoesmcartor
Copy link

@Aljullu Ahh perfect. Thank you so much! I will use the workaround for now. I think I was just missing the "!important" part and it was blocking me. Still learning over here!

I'll keep an eye on this issue just in case you decide to pass the placeholder and wrapper props. Thanks again!

@BilyachenkoOY
Copy link

@Aljullu May I raise a PR with wrapperProps?

@Aljullu
Copy link
Owner

Aljullu commented Aug 7, 2019

@Aljullu May I raise a PR with wrapperProps?

Sure, that would be awesome! There is a WIP PR about adding loadedImageProps and placeholderProps which might be useful to use as a reference: #46.

You can even use the add/loaded-image-props branch as a base if you want. master would be fine as well.

@BilyachenkoOY
Copy link

@Aljullu, yeah I missed #46. I think placeholderProps should be enough

@johnstonbl01
Copy link
Contributor

@Aljullu 👋 Hey there! Would it be OK if I worked on the wrapperProps PR?

@Aljullu
Copy link
Owner

Aljullu commented May 12, 2020

Sure thing @johnstonbl01! I'm happy to review any new PRs that are pushed to the repo. Make sure to take a look at #46 in case you think that might be good enough for your use-case. Otherwise maybe it's a good base to start from. It has got big conflicts but I think it's just formatting (spaces → tabs).

@johnstonbl01
Copy link
Contributor

Will do - thanks!

@Aljullu
Copy link
Owner

Aljullu commented May 29, 2020

A beta version with the new wrapperProps prop is available in NPM. You can get it from:

Thanks @johnstonbl01 for the contribution.

@JayBox325
Copy link

@Aljullu Thanks for adding this, can you please add an example to the docs for how to use the wrapperProps object as I haven't been able to get it working with different tries. Thanks

@Powerinno
Copy link

It still has an inline-block display but, it is not in a CSS class, is inline style. Can anyone show me how could I remove it? https://github.com/Aljullu/react-lazy-load-image-component/blob/master/src/components/LazyLoadImage.jsx - line 115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants