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

add onWheel prop #778

Merged
merged 2 commits into from
Aug 18, 2017
Merged

Conversation

implausible
Copy link
Contributor

This is a follow up to #541.

The PR simply exposes the onWheel prop for Grid.

@bvaughn
Copy link
Owner

bvaughn commented Aug 15, 2017

I'm not sure I want to expose a "wheel" event handler on Grid. While scroll events are async, wheel and touch* events are sync (at least by default) and so can cause performance problem.s

@implausible
Copy link
Contributor Author

Well, without the ability to force react-virtualized to run wheel events synchronously, react-virtualized has huge tearing issues in electron. We talked about this previously, what would you suggest we do now?

@implausible
Copy link
Contributor Author

Perhaps you could modify this approach to enable your onWheel handler to be injected in? eg what if Grid exposed a new onWheel: PropTypes.func (optinoal) property that enabled you to inject your own throttled wheel handler? This way if it works well for your use-case you could continue to use it without potentially causing regressions for existing apps.

This is where we left off last time, we haven't had the need to update react-virtualized in awhile, but we're planning on updating react-virtualized, soon. Before we can update react-virtualized, we need to make sure that we can fix the tearing that occurs in multiple grid scrolling. Since this seemed to be the direction we wanted to go last time, if it's no longer the direction you intend, what do we do now?

@bvaughn
Copy link
Owner

bvaughn commented Aug 15, 2017

Don't know. Need to think about it. Not immediately comfortable with encouraging usage of "wheel" events (with an explicit onWheel prop) since they have known perf impact. (Also there are no other top-level event handlers, excluding onScroll, which is a custom thing and not a pass-thru for the built-in "scroll" event.

Maybe some kind of rest prop that just lets you pass through unfiltered properties to the root <div> element created by Grid.

You're right that I mentioned onWheel as a possible solution a few months back. 😄

@implausible
Copy link
Contributor Author

I'll revise for a rest prop.

@implausible implausible force-pushed the feature/enable-on-wheel branch from d87aeac to 437bd5f Compare August 15, 2017 19:16
@bvaughn bvaughn merged commit 9f3e81d into bvaughn:master Aug 18, 2017
@athomann
Copy link
Contributor

Also was looking for an onWheel prop. We are attempting to prevent the parent container from scrolling when you reach the bottom of the virtual list. So far I can only implement that with onWheel, unless you know of another way?

@athomann
Copy link
Contributor

Nevermind, I should be able to solve it when this is released.

@bvaughn
Copy link
Owner

bvaughn commented Sep 18, 2017

This feature has been released in version 9.10.0. Thank you for contributing!

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