[QUESTION]: lazyload-wrapper class

Question

[QUESTION]: lazyload-wrapper class

simplecommerce opened this issue 4 years ago · 33 comments

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!

Answer 1 · 2020-06-18T13:56:37.000Z

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

Answer 2 · 2020-06-18T14:05:47.000Z

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

Answer 3 · 2020-06-18T14:11:37.000Z

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

Answer 4 · 2020-06-18T14:12:24.000Z

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

Answer 5 · 2020-06-18T14:18:04.000Z

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

Answer 6 · 2020-06-18T20:40:49.000Z

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

Answer 7 · 2020-06-19T05:16:02.000Z

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

Answer 8 · 2020-06-19T05:16:18.000Z

will keep this open for few more weeks

Answer 9 · 2020-06-20T00:34:05.000Z

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.

Answer 10 · 2020-06-21T07:30:58.000Z

@gilbarbara did it break your production too?

Answer 11 · 2020-06-21T14:31:07.000Z

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.

Answer 12 · 2020-06-21T15:07:41.000Z

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

Answer 13 · 2020-06-26T20:38:04.000Z

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"

Answer 14 · 2020-06-27T07:49:54.000Z

@kbradley that is a really good point thanks

Answer 15 · 2020-06-28T00:29:52.000Z

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

Answer 16 · 2020-06-28T09:33:54.000Z

@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

Answer 17 · 2020-06-28T10:08:23.000Z

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

Answer 18 · 2020-06-28T18:25:12.000Z

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

Answer 19 · 2020-06-29T05:05:25.000Z

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

Answer 20 · 2020-06-29T18:08:42.000Z

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!

Answer 21 · 2020-07-13T02:46:59.000Z

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 ?

Answer 22 · 2020-07-13T20:02:07.000Z

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

Answer 23 · 2020-07-16T20:10:35.000Z

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>
  );
}

Answer 24 · 2020-07-17T05:06:26.000Z

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

Answer 25 · 2020-07-29T18:08:20.000Z

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

Answer 26 · 2020-07-29T18:41:49.000Z

@EricMCornelius yes this is scheduled for next release

Answer 27 · 2020-09-12T06:29:11.000Z

Any update on this next release?

Answer 28 · 2020-12-15T12:45:34.000Z

+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

Answer 29 · 2021-02-05T16:40:09.000Z

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

Answer 30 · 2021-07-23T05:48:53.000Z

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?

Answer 31 · 2021-08-12T08:25:25.000Z

I have a strange issue, I have a long list in a popup-like dropdown, so I have set the overflow property. Now this dropdown might not be visible complete while mounting on the viewport and the bottom of this can be hidden behind let's say by a mobile keyboard. Then only those lazyload items are visible which are not behind the keyboard and visible on the screen, but when I scroll my page to bring the dropdown completely visible on screen, the lazyload items are not reloaded but when I scroll inside the container, they are reloaded. I have tried everything but was not able to visible all components present in the container viewport instead of the window viewport at the time of mounting.
Any solution for that?

Answer 32 · 2021-09-13T07:58:21.000Z

@ameerthehacker

@EricMCornelius yes this is scheduled for next release

Apologies if I've missed this in the documentation, but I can't find any way to change the div to a span. I am also the use case of having img elements within p elements, and div elements should not appear within p elements.

Answer 33 · 2022-05-25T08:01:16.000Z

Hi @ameerthehacker, Thanks for your work in the library.
I use this library but I have an error when I installed 'react-lazy-load'.
I found 'npm install --save react-lazyload' command, so I only used it but
I don't know why isn't it working.
Would you mind helping me solve this problem.
thanks.

'Problem↓'
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: music@0.1.0
npm ERR! Found: react@18.1.0
npm ERR! node_modules/react
npm ERR! react@"^18.1.0" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer react@"^0.14.0 || ^15.0.0-0 || ^16.0.0" from react-lazy-load@3.1.13
npm ERR! node_modules/react-lazy-load
npm ERR! react-lazy-load@"*" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /Users/MyName/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR! /Users/MyName/.npm/_logs/2022-05-25T07_38_50_239Z-debug-0.log