Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scrollContainer does not work when there are two or more scrollContainers on a page containing LazyLoads #371

Open
JohnThomson opened this issue Jan 13, 2022 · 3 comments

Comments

@JohnThomson
Copy link

JohnThomson commented Jan 13, 2022

See https://codesandbox.io/s/johnt-tc375-tc375?file=/src/index.js. Observe that the top container works properly, but when the bottom container is scrolled, the placeholders become visible and are never converted to show their children. (Unless you scroll the top container some more...it gets the listener for ALL the LazyLoad components.)

You may say that this case of containers that scroll is supposed to be handled by setting the overflow property of the LazyLoad items. If you change the sandbox I provided to do this, none of the LazyLoads ever show their children, in either parent.

I have studied the source and can see what is wrong, but it is less obvious how to fix it.

In the case of scrollContainer, the problem is in LazyLoad.componentDidMount in what is currently (3.2.0) line 276:

    } else if (listeners.length === 0 || needResetFinalLazyLoadHandler) {

This line appears to be left over from when the only possible scrollport was the window, and a listener only needed to be added once. But if different LazyLoad items have different scrollContainers they will compute different scrollports (line 236) and each scrollport needs the listener added to it; but the test for listeners.length being zero prevents one from being added to any scrollContainer but the first.

In the case where overflow is used, the block starting at line 267 completely ignores scrollContainer if overflow is true. Instead, a function scrollParent is used to try to figure out what the scrollContainer should be. In my example, scrollParent fails because it is looking for an element where overflow is set to either scroll or auto in BOTH directions; but mine only scroll vertically, and have overflowX set to hidden.

I think the right answer is that if scrollContainer is set, the block currently coded for overflow should be used, but the listener should be added to the already-computed scrollport rather than a block found by scrollParent(). scrollParent() should only be used if overflow is set and scrollContainer is not. It would be helpful to clarify in the doc that overflow is a shortcut for automatically deducing scrollContainer that only works if it scrolls in both directions. But it's not obvious how the cleanup code in componentWillUnmount should be changed...currently, it doesn't appear to remove a scrollContainer listener at all.

Another option would be for overflow to specify a value indicating what direction(s) the scrolling block scrolls. Then scrollParent could be coded to find it correctly.

Another issue with overflow is that if overflow is specified, a resize listener is never added; but (as my example also shows) it may be needed, since the scrolling blocks may be set to change size with the window.

For now I plan to work around this by adding my own listener to the scroll event of the appropriate divs and using forceCheck. But it would be nice if this could be fixed, or at least these limitations documented.

@JohnThomson
Copy link
Author

That workaround was not very satisfactory, because using scrollContainer visibility is evaluated based on whether the item is in the window, not on whether it's in the scrollContainer; and using overflow, it's finding the wrong parent.
Instead, I set up a temporary patch that makes scrollParent only consider the overflowY value. This works for me, but is not satisfactory as a PR because it would mess up anyone who wants a laziness in a parent that scrolls horizontally or bidirectionally.

JohnThomson added a commit to JohnThomson/BloomDesktop that referenced this issue Jan 14, 2022
- See twobin/react-lazyload#371. This workaround assumes the proper scrolling container for a LazyLoad element can be found by searching for the closest parent with overflowY "auto" or "scroll", ignoring overflowX. Therefore it is only suitable for lazy components in things that scroll vertically. The commit also adds the build support we need to use the patch.
@ErnestBrandi
Copy link

I have a similar issue. Just using a "class type" identifier, and therefore multiple scroll containers, and none of them get updated.

@volodymyr-kryvoshapov
Copy link

volodymyr-kryvoshapov commented Oct 26, 2023

Similar issue, it is simple to reproduce. Just copy containers in overflow.js file, run server and try to scroll:

import React, {Component} from 'react';
import LazyLoad from '../../src/';
import Widget from '../components/Widget';
import Operation from '../components/Operation';
import {uniqueId} from '../utils';

export default class Overflow extends Component {
  constructor() {
    super();

    const id = uniqueId();
    this.state = {
      arr: Array.apply(null, Array(20)).map((a, index) => {
        return {
          uniqueId: id,
          once: [6, 7].indexOf(index) > -1
        };
      })
    };
  }

  handleClick() {
    const id = uniqueId();

    this.setState({
      arr: this.state.arr.map(el => {
        return {
          ...el,
          uniqueId: id
        };
      })
    });
  }

  render() {
    return (
      <div className="wrapper overflow-wrapper">
        <Operation type="overflow" onClickUpdate={::this.handleClick} />
        <h1>LazyLoad in Overflow Container</h1>
        
        <div id="www" className="widget-list overflow">
          {this.state.arr.map((el, index) => {
            return (
              <LazyLoad scrollContainer="#www" once={el.once} key={index} overflow throttle={100} height={200}>
                <Widget once={el.once} id={el.uniqueId} count={ index + 1 } />
              </LazyLoad>
            );
          })}
        </div>


        <div id="xxx" className="widget-list overflow">
          {this.state.arr.map((el, index) => {
            return (
              <LazyLoad scrollContainer="#xxx" once={el.once} key={index} overflow throttle={100} height={200}>
                <Widget once={el.once} id={el.uniqueId} count={ index + 1 } />
              </LazyLoad>
            );
          })}
        </div>


        <div id="yyy" className="widget-list overflow">
          {this.state.arr.map((el, index) => {
            return (
              <LazyLoad scrollContainer="#yyy" once={el.once} key={index} overflow throttle={100} height={200}>
                <Widget once={el.once} id={el.uniqueId} count={ index + 1 } />
              </LazyLoad>
            );
          })}
        </div>
      </div>
    );
  }
}


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants