[QUESTION]: lazyload-wrapper class

[simplecommerce]

Hi @ameerthehacker, in the latest commit, you merged a pull request that added this span with the className lazyload-wrapper.

Is there a reason for this?

The previous behavior when the children was visible, was not to have any placeholder or wrapper around them.

Now because of this commit, it breaks my layout because of the extra span element with that class.

Is this the intended behavior?

Previous code:

return this.visible ? this.props.children : this.props.placeholder ? this.props.placeholder : _react2.default.createElement('div', { style: { height: this.props.height }, className: 'lazyload-placeholder' });

Now:

return _react2.default.createElement(
        'span',
        { className: 'lazyload-wrapper', ref: this.setRef },
        this.visible ? this.props.children : this.props.placeholder ? this.props.placeholder : _react2.default.createElement('div', {
          style: { height: this.props.height },
          className: 'lazyload-placeholder'
        })
      );

Please let me know, thanks!

[ameerthehacker]

Hi @simplecommerce this span is necessary to avoid usage of findDom API and ReactRef actually needs a dom element in the jsx, the className in the span tag is provided for the same purpose for you to change its styling. Hope this clarifies that

[simplecommerce]

@ameerthehacker yes I understand, this change causes me problems because the previous behavior allowed us to apply styles to children without having to think of there is going to be an extra div/span around them, in the case of nesting of the lazy load component, it makes it impossible to know how to style it unless you know how the code was used, since the CMS we use it with is used by the customers.

is there a huge performance gain for this reason for the switch?

[ameerthehacker]

@simplecommerce the findDom was deprecated and it was causing warning in lot of productions that is why we switched to React Ref. I can understand your concern that the consumers manipulate the code for their needs, in those cases I would suggest two thins. Try to add some styling which nullifies the span tag

.lazyload-wrapper {
...
}

or please switch to the previous version which does not break your code.

[ameerthehacker]

@simplecommerce for your reference #303
we ran a beta and then switched to stable but I honestly did not anticipate this

[simplecommerce]

@ameerthehacker yes that is what I am doing for the moment, I will try to figure out if there is a way I can re-factor my code to include this new approach, thanks!

[danielpquinn]

@ameerthehacker Thanks for your work on this library. Unfortunately, the change from 2.6.7 to 2.6.8 broke layouts on our site too due to the new wrapper element. Does it make sense to use a major version change here? I imagine there will be a lot of people who are surprised by this behavior and end up with broken CSS selectors.

[ameerthehacker]

@danielpquinn I'm also starting think on the same line, if more people are affected I will make this into a Major update

[ameerthehacker]

will keep this open for few more weeks

[gilbarbara]

Thanks for updating it!

But, I think a major version is in order. I didn't expect tons of snapshots to break in a patch update.

[ameerthehacker]

@gilbarbara did it break your production too?

[gilbarbara]

Hey @ameerthehacker

I reverted to the previous version since it broke my snapshots.
But it would definitely break the UI to have a wrapping span since I have styling applied to the immediate children.

[ameerthehacker]

@gilbarbara thanks for letting me know, will revert back to old version and will make this as major