[QUESTION]: lazyload-wrapper class

This issue has been tracked since 2020-06-18.

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 wrote this answer on 2020-06-18

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 wrote this answer on 2020-06-18

@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 wrote this answer on 2020-06-18

@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 wrote this answer on 2020-06-18

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

simplecommerce wrote this answer on 2020-06-18

@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 wrote this answer on 2020-06-18

@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 wrote this answer on 2020-06-19

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

ameerthehacker wrote this answer on 2020-06-19

will keep this open for few more weeks

gilbarbara wrote this answer on 2020-06-20

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 wrote this answer on 2020-06-21

@gilbarbara did it break your production too?

gilbarbara wrote this answer on 2020-06-21

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 wrote this answer on 2020-06-21

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

kbradley wrote this answer on 2020-06-26
10

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 wrote this answer on 2020-06-27

@kbradley that is a really good point thanks

Slapbox wrote this answer on 2020-06-28

@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 wrote this answer on 2020-06-28

@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 wrote this answer on 2020-06-28

@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.

Slapbox wrote this answer on 2020-06-28

@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 wrote this answer on 2020-06-29

@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?

Slapbox wrote this answer on 2020-06-29

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 wrote this answer on 2020-07-13

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 ?

Slapbox wrote this answer on 2020-07-13

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

eddyw wrote this answer on 2020-07-16

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 wrote this answer on 2020-07-17

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 wrote this answer on 2020-07-29

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 wrote this answer on 2020-07-29

@EricMCornelius yes this is scheduled for next release

cwagner22 wrote this answer on 2020-09-12

Any update on this next release?

rohan-buchner wrote this answer on 2020-12-15

+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 wrote this answer on 2021-02-05

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 wrote this answer on 2021-07-23

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?

More Details About Repo
Owner Name twobin
Repo Name react-lazyload
Full Name twobin/react-lazyload
Language JavaScript
Created Date 2015-08-07
Updated Date 2022-11-29
Star Count 5639
Watcher Count 52
Fork Count 482
Issue Count 153

YOU MAY BE INTERESTED

Issue Title Created Date Updated Date