caseywebdev / react-list

:scroll: A versatile infinite scroll React component.
https://caseywebdev.github.io/react-list
MIT License
1.96k stars 176 forks source link

Add new property "window" #208

Closed leventebalogh closed 6 years ago

leventebalogh commented 6 years ago

The Problem We have kind of an edge-case: we have a React app which runs inside an iframe, and we would like to have the infinite scroll to work like if it was part of the top window.

What changed? Introduced a new prop on the component called window which let's you overwrite the window object used. It defaults to the global window object.

caseywebdev commented 6 years ago

Is there a specific reason scrollParentGetter doesn't work?

leventebalogh commented 6 years ago

Thanks for the fast reply dude!

Yea, so the reason is that even if you pass in the top window - scrollParentGetter={ () => window.top } - it doesn't work because in some other places it's depending on the window object of the iframe itself.

Because of that it doesn't recognise the scrollParent as being a window object (window and window.top differs), so certain parameters get calculated wrongly. E.g. size becomes NaN and so on.

I hope I could explain it correctly, but if that's not the case and you would answers just shoot :)

So summing it up this PR is just giving aid for an edge-case and leaves normal scenarios untouched.

caseywebdev commented 6 years ago

I see. I'd like to keep the API as small as possible. What if instead of the scrollParent === window checks they were scrollParent instanceof Window? Then we would use the correct property getters for a Window instance when appropriate.

leventebalogh commented 6 years ago

The problem is that the code uses the window object (and the document below it) in a few other places as well which are needed for the correct calculation. So with only that change it wouldn't work :(

Understand your concern absolutely, we have chosen this library because of it's simpleness. But still I have the hope :D

caseywebdev commented 6 years ago

Well I meant updating those spots to match ;)

Can you try setting scrollParentGetter={() => (your window)} in this branch?

leventebalogh commented 6 years ago

Aaa I got what you mean, sorry mate. I am checking the branch, something is not working but I think we can take it there with your approach. Debugging it and will get back soon!

leventebalogh commented 6 years ago

So checked it and I have only two notes:

1. react-list.es6 #194 Go to line We would need to get the document object from the scrollParent if it is a window object here as well:

getScrollSize() {
    const {scrollParent} = this;
    const _document = scrollParent instanceOf Window ? scrollParent.document : document;
    const {body, documentElement} = _document
    // ...

2. scrollParent instanceof Window doesn't work It surprised me, but although window instanceof Window gives back true, two following two are returning a false, so the current way of detecting window objects is not working unfortunately :(

What if we would do one the following instead? I like the first better to be honest, for the second we could use any other property obviously, but that seems a bit more hacky to me.

Thanks again for your time mate.

caseywebdev commented 6 years ago

It surprised me, but although window instanceof Window gives back true, two following two are returning a false

What browser did you see this in? window.top and window.parent both are both instanceof Window in Safari, Chrome and FF in my testing.

leventebalogh commented 6 years ago

Chrome. Yea, the reason why you see it like that is because window = window.top in your case - as you run the script in the main document. If you would run it from an iframe though, it wouldn't work.

It's all right though, as we figured out other problems as well with this iframe approach, it seems it just was a dead end (not because of this), so maybe we should actually just close this PR, it's a very rare edge-case anyway. Thanks a lot for your help though ;)

caseywebdev commented 6 years ago

Gotcha. Happy to attempt to help 😉