Add wrapperProps for <span>

[johnstonbl01]

Fixes #33

Description

This PR adds the ability to pass a props object to the wrapping <span> element when using effects or placeholders.

Here's an example of this change in action:

Code for above in a CRA app:

App.js:

import 'react-lazy-load-image-component/src/effects/opacity.css';
import React from 'react';
import { LazyLoadImage } from 'react-lazy-load-image-component';

import imgSrc from './spiderman.jpg';
import './App.css';

function App() {
  return (
    <div className="App">
      <header className="App-header">
        <div className="card">
          <LazyLoadImage
            height={150}
            src={imgSrc}
            style={{
              maxWidth: '100%',
              height: 'auto',
              maxHeight: 150,
              alignSelf: 'center',
            }}
            effect="opacity"
            wrapperProps={{
              style: {
                height: 200,
                display: 'inline-flex',
                border: '2px solid blue',
              },
            }}
          />
        </div>
      </header>
    </div>
  );
}

export default App;

Let me know if there's anything that I need to change!

[Aljullu]

Awesome, thanks for working on that @johnstonbl01!

I tested it with <LazyLoadImage> and it works well but I noticed a small issue: wrapperProps seem to be added not only to the wrapper but also to the placeholder span:

I left an inline comment where I think the issue is. Let me know if it makes.

[Aljullu]

I don't think wrapperProps is needed here. This is the placeholder span, which is different from the wrapper span. The placeholder span is wrapped by the wrapper span and it gets replaced once the component or image are loaded. Hope it's not too confusing. 🙂

[johnstonbl01]

Cool - I will get this changed this weekend. Thanks for the feedback!

[johnstonbl01]

@Aljullu

Question about a bit of the code. What's the reasoning behind:

if ((!effect && !placeholderSrc) || visibleByDefault) {
  return lazyLoadImage;
}

It looks like all the other use cases get wrapped in the <span/> except for the ones that fall inside that conditional. I'm wondering if someone is using a placeholder and would expect it to also be wrapped in the span and have the wrapper props applied? If not, then I can just add a note in the documentation for that. If those elements should also wrap the placeholder span, then I can make that update too.

I just wanted to get your thoughts before making any code changes. ✌️

[Aljullu]

It looks like all the other use cases get wrapped in the <span/> except for the ones that fall inside that conditional.

That's correct, the wrapper is only needed to display the placeholder image or when there is an effect. Otherwise, there is no need to render it.

I'm wondering if someone is using a placeholder and would expect it to also be wrapped in the span and have the wrapper props applied? If not, then I can just add a note in the documentation for that.

Yeah, we should make it clear the wrapper is only rendered when placeholderSrc or effect props are defined. Checking the docs, wrapperClassName already has an explanation for it, but we could expand it if it's not clear.

Hope that helps @johnstonbl01.

[Aljullu]

Thinking about it twice, to reduce confusion it might make sense to rewrite the check you posted as:

const needsWrapper = (effect || placeholderSrc) && !visibleByDefault;
if (!needsWrapper && !wrapperClassName && !wrapperProps) {
	return lazyLoadImage;
}

So if wrapperClassName or wrapperProps are defined, the wrapper is rendered. I think that might be less confusing, thoughts?

Feel free to handle it in this PR or I can open a new one.

[johnstonbl01]

That makes sense to me. I'll take care of it in this PR - thanks for the clarification! I should be able to get to this tomorrow. :)

[johnstonbl01]

Ok - should be good to go now on these changes.

• Removed wrapperProps from being passed to the placeholder
• Updated the conditional to match your above suggestion (I agree it's easier to read)
• Updated the docs for the wrapperProps to reflect that the wrapping span is rendered when using placeholderSrc and effect

If I missed anything, please let me know!

[Aljullu]

Working like a charm, @johnstonbl01. Thank you! I will merge it now and get a beta version released soon.

[johnstonbl01]

Working like a charm, @johnstonbl01. Thank you! I will merge it now and get a beta version released soon.

Anytime! Thanks for the opportunity to help and contribute!