-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fixes https://github.com/azuqua/react-dnd-scrollzone/issues/19 #20
base: master
Are you sure you want to change the base?
Conversation
Strange, turns out scrollingElement has differences in scroll/height/width calculations between chrome 59 and 61 as well, making it unreliable. Changed to the more stable Math.max(document.documentElement.clientHeight, window.innerHeight || 0) measurements instead of the container properties. |
I'm hesitant to merge something like this in because it seems out of scope for this project. But this is actually the second PR that adds behavior like this so I guess there is demand. Could you add a new example in the |
The way I see it it's about consistency. If you remove the width, height and overflow css from .App container (from the basic example) the scrolling component no longer works. From the point of view of a user that doesn't look at the code it should work on any scrolling element in the same manner. Why should this particular scrolling container be outside the scope? As a future improvement, if you decide to adopt the change, scrollingElement prop should no longer be used and the scroll applied automatically if scroll is on scrollingElement. In any case, thank you for the work you put into it and keep it up. I went with using the modified sources so there is no hurry with the merge. |
Removed console.logs
I've been using this pull request on my own site, but I noticed that it stopped my page from loading in IE 11. After triggering the first It has to do with this line:
After running that, the next line:
causes an error in IE:
I haven't done a full fledged investigation yet, but letting you know what I know so far. |
@mreishus The example i added (scrollingElement) works just fine on IE11 (i just checked). Are you sure it's not something on your side? Please check and let me know. If you have a public page i could access where the issue reproduces, it would be easier. |
Unfortunately, it's a large corporate app, so I have no easy way to show public access. However, I did a simple check by running |
hmm, yea, polyfilling it makes it work for IE in me ( https://github.com/mathiasbynens/document.scrollingElement ) |
Simply set scrollingElement on ScrollingComponent and the scroll will also work on document.scrollElement. As mentioned in the initial issue https://github.com/mathiasbynens/document.scrollingElement can be used if support is needed for older browsers.