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

Fixes https://github.com/azuqua/react-dnd-scrollzone/issues/19 #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alex7071
Copy link

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.

@alex7071
Copy link
Author

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.

@nickclaw
Copy link
Contributor

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 examples/ folder so I can understand the use case and test it out easily?

@alex7071
Copy link
Author

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.
Added scrolling example, but needs to use the modified build. As said, it's just the basic example without the height/width/scroll css and with more elements.
As for using a container in the application (which you provided as a solution in previous closed issues) there are layouts in which this solution cannot be used. If you have a layout with a fixed right: 0 container with scroll on the right side and you try using a scrolling main container instead of scrollingElement, the two containers will have the scroll bar one on top of the other, looking very strange to say the least. ScrollingElement is the only element that has the scroll outside the viewport so right:0 can be used without scrollbars overlapping. See issue exemplified in the image below.

image

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
@mreishus
Copy link

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 .render() call in react, an error occurs.

It has to do with this line:

+ this.container = this.props.scrollingElement ? document.scrollingElement : findDOMNode(this.wrappedInstance);

After running that, the next line:

this.container.addEventListener('dragover', this.handleEvent);

causes an error in IE:

Unable to get property 'addEventListener' of undefined or null reference.

I haven't done a full fledged investigation yet, but letting you know what I know so far.

@alex7071
Copy link
Author

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

@mreishus
Copy link

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 document.scrollingElement in the browser console of chrome, firefox, and IE. It returns an element in chrome and firefox, but not IE. Maybe you have a polyfill or something filling it in?

iexplore_2018-01-24_13-20-32

@mreishus
Copy link

mreishus commented Jan 24, 2018

hmm, yea, polyfilling it makes it work for IE in me ( https://github.com/mathiasbynens/document.scrollingElement )

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

Successfully merging this pull request may close these issues.

3 participants