Add loadedImageProps and placeholderProps props #46
Conversation
Aljullu
force-pushed
the
add/loaded-image-props
branch
from
f0a3a32
to
9fc73b0
Compare
Aljullu
changed the title
| const props = { | ||
| ...placeholderProps, | ||
| style: { | ||
| backgroundImage: loaded ? '' : 'url( ' + placeholderSrc + ')', |
There was a problem hiding this comment.
Maybe it will be better to put these styles somewhere in the css, so that it will be enough to pass className in the placeholderProps (and merge classes here) to override styles without important hacks
There was a problem hiding this comment.
Also it will simplify applying different styles for different resolutions using media queries
There was a problem hiding this comment.
Thanks for taking a look @BilyachenkoOY ! The issue is that we need to read placeholderSrc from the props, and that can't be done if we move the styles completely to a CSS file. It's true we could keep backgroundImage in the style while moving everything else to a CSS file, but I think that would be confusing.
In the case you need to make a lot of modifications to the placeholder style, probably it's a better idea passing a node as the placeholder prop instead of adding props to the default placeholder. How does that sound?
There was a problem hiding this comment.
I still believe that there should be as few as possible in props and it will be fine if there will be only backgroundImage + height&width since it should be as easy as possible to override styles using wrapperClassName.
As for placeholder , it will be placed inside this <span> and is not suitable for all style manipulations.
It is not a big issue to me to add few important directives to my CSS, so it is up to you
Fixes #31.
Description
loadedImageProps: it allows passing an object of props that will be applied to the loaded image.placeholderProps: it allows passing an object of props that will be applied to the placeholder.