-
Notifications
You must be signed in to change notification settings - Fork 489
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
Comments
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. |
- 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.
I have a similar issue. Just using a "class type" identifier, and therefore multiple scroll containers, and none of them get updated. |
Similar issue, it is simple to reproduce. Just copy containers in overflow.js file, run server and try to scroll:
|
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:
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.
The text was updated successfully, but these errors were encountered: