[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 https://github.com/twobin/react-lazyload/issues/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

[kbradley]

Could you consider also at least changing the tag from a span to a div?

Span tags are inline-level elements and should only contain "phrasing content". If they are used to wrap block-level elements such as div tags then the HTML structure is invalid. In many usage cases users will end up wrapping block-level content which will result in an invalid HTML structure.

https://html.spec.whatwg.org/#the-span-element https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Phrasing_content

https://developer.mozilla.org/en-US/docs/Web/HTML/Inline_elements "Content model Generally, inline elements may contain only data and other inline elements. You can't put block elements inside inline elements."

https://en.wikipedia.org/wiki/Span_and_div "For example, regardless of CSS, a span element may not contain block-level children"

[ameerthehacker]

@kbradley that is a really good point thanks

[Nantris]

@ameerthehacker Thanks for the explanation!

Is there no way to remove the wrapper upon completing the lazy load, at least for cases where once={true}? There's a number of downsides for end users, though they're not deal-breaking.

We're trying to reduce our total DOM node count and depth, and so this adds hundred or more elements to our page. The recommendation is to keep total DOM nodes to 1,500 or less, so that's nearly 7% of the total. It's not like 1500 is a brick wall, but it would be highly beneficial for us if we could somehow remove the wrapper once the load is complete.

The main thing we use React-Lazyload for actually, is reducing DOM nodes, sort of like an easy to implement alternative to React-Waypoint. I imagine this is not the way most people are using it, but the DOM node count concern remains.

While I'm writing this anyway, I want to also speak up in support of changing the span to a div.

Thanks for your great work @ameerthehacker!

[ameerthehacker]

@Slapbox thanks for your inputs, I totally agree with you and I'm currently looking into ways by which we can totally eliminate the extra node itself, will keep posting the updates

[ameerthehacker]

@simplecommerce @gilbarbara I have reverted back the old change as 2.6.9 and have released the breaking change as 3.0.0 to npm, @kbradley I have switched span tag with div. @Slapbox currently I'm not sure how to avoid the extra DOM node overhead but will keep thinking on it and meantime if you find any way to improve it feel free to raise a PR.

[Nantris]

@ameerthehacker thanks for your speedy replies and updates!

As far as avoiding the extra DOM node before loading the content, I don't see any way right now - but I'm not clear on what purpose the wrapper serves once the content has finished loading (at least when once={true}. Can you help me understand the benefit at that stage?

[ameerthehacker]

@Slapbox no when we use once={true} I don't see any purpose for the div tag but copying the children and replacing div tag will make the performance even bad?

[Nantris]

copying the children and replacing div tag will make the performance even bad?

Most likely it would be worse for performance, yeah. I just wanted to make sure I understood the purpose of the wrapper before trying to offer any ideas (if any do occur to me.)

Thanks again @ameerthehacker!

[gittestfor]

Hi I can't find a prop for LazyLoad so it knows the array's length so it doesn't show the placeholder when it reaches the end of the array . How can I tell it dont show the placeholder={

loading...

} when you've reached the end of the array ?

[Nantris]

@gittestfor it doesn't seem like your comment is related to the topic of this issue thread. Maybe I'm misunderstanding.

[eddyw]

Hi, I just want to say that ... The wrapper is a terrible idea 😅

Making it a div is even worse. For example, div isn't valid within p (where you'd expect to have an img, for example). A solution to this problem could be to allow the component to accept a prop as, so the wrapper could be defined as any element, block or inline.

Here is my quick attempt at finding an alternative to findDOMNode that may be helpful .. or inspire a better idea 😅

const LazyThing: React.FC = ({ children }) => {
  const refDOMNode = React.useRef<Element | null>();

  const transformToComment = (e: HTMLSpanElement) => {
    if (!e) return;
    const comment = document.createComment(e.innerHTML);
    e.parentNode?.replaceChild(comment, e);
    return comment;
  };

  const findDOMNode = (e: HTMLSpanElement) => {
    if (!e) return;

    const comment = transformToComment(e)!;

    if (
      comment.nextElementSibling?.nodeType !== 8 &&
      comment.nextElementSibling?.textContent !== "END"
    ) {
      refDOMNode.current = comment.nextElementSibling;
      console.log("findDOMNode:", refDOMNode.current); // <<< ta-da!
    } else {
      refDOMNode.current = null;
    }
  };

  return (
    <>
      <span key="s" ref={findDOMNode}>
        START
      </span>
      {children}
      <span key="e" ref={transformToComment}>
        END
      </span>
    </>
  );
};

export default function App() {
  return (
    <div className="App">
      <LazyThing>
        <div>Example</div>
      </LazyThing>
    </div>
  );
}

[ameerthehacker]

Thanks for the feedback @eddyw, your idea on the first look seems great, can you please raise a PR to do the same and if it works I will get it released

[EricMCornelius]

At the very least, I think we need to be able to configure the selected wrapper element type, This change breaks even straightforward table row lazy loading...

[ameerthehacker]

@EricMCornelius yes this is scheduled for next release

[cwagner22]

Any update on this next release?

[rohan-buechner]

+1 From my side... Out app is using react-lazyload as well... and the wrapper just caused a whole world of pain... I'm reverting back to 2.6.5 for now

[cheunghoman2]

Thank you so much Ameerthehacker for your contribution!

Sorry that I am new to react so may ask a stupid question. I applied "react-lazyload" in my app. It works for lazy loading a component containing img (map loop). However, when I add css (float: left;) to my app's component, the lazy loading not work (it will load all the img at the same time)!

Am I missing to use some Props? (I just use height and width and it has no issue if I don't use float: left)

And I also check the element in Chrome Debug mode, just found NO height at all for "lazyload-wrapper". And the height will appear if no "float: left" is used...

[ykhoe]

At the very least, I think we need to be able to configure the selected wrapper element type, This change breaks even straightforward table row lazy loading...

Do you have workaround for this issue?