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 prop for disabling the wheel handler #97

Merged
merged 3 commits into from
Jan 9, 2017
Merged

add prop for disabling the wheel handler #97

merged 3 commits into from
Jan 9, 2017

Conversation

httnn
Copy link
Contributor

@httnn httnn commented Dec 20, 2016

Description

adds a new boolean prop disableWheelHandler, that, when enabled, will disable handling wheel events.
adds a new boolean prop onVerticalScroll, that works like onHorizontalScroll.

Motivation and Context

we have a use-case in which we need to handle the wheel by ourselves since we are using window scrolling with the table. now we need to resort to a hack like this:

<Table
  ref={e => e && (e._wheelHandler = () => {})}
...

which is very ugly, so would be nice to be able to do this "officially" via a property!

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@KamranAsif
Copy link
Contributor

I think a better solution is extending onVerticalScroll to stop propagation if it returns false.
We did a change for onHorizontalScoll here: https://github.com/schrodinger/fixed-data-table-2/blob/master/src/FixedDataTable.js#L1136

Thoughts?

@@ -346,7 +351,7 @@ var FixedDataTable = React.createClass({
},

_shouldHandleWheelX(/*number*/ delta) /*boolean*/ {
if (this.props.overflowX === 'hidden') {
if (this.props.overflowX === 'hidden' || this.props.disableWheelHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to handle ReactWheelHandler being initalized here:
https://github.com/schrodinger/fixed-data-table-2/blob/master/src/FixedDataTable.js#L334

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? ReactWheelHandler takes both _shouldHandleWheel* functions as parameters, and since it is initialised in componentWillMount, changing disableWheelHandler during the lifecycle of the component wouldn't change any behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! why doesn't this wheelHandler use the same _shouldHandleWheel* functions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure (we inherited this code from the original repo)
I'm guessing one reason is that it doesn't have access to this.props yet.

@wcjordan This seems like a big memory leak, creating unnecessary wheel handlers if the table switches from overflow on and off.

It seems unnecessary to create it in that function, since the handler is already initialized in componentWillMount.

@httnn
Copy link
Contributor Author

httnn commented Dec 21, 2016

wouldn't changing onVerticalScroll also disable scroll bars? this prop would only be concerned with blocking the wheel event from being listened to.

@KamranAsif
Copy link
Contributor

No, overflowX/overflowY set to none disables the scrollbars.
Heres an example of how it works for horizontal scrolling:
https://jsfiddle.net/ffjxxkee/

@httnn
Copy link
Contributor Author

httnn commented Dec 30, 2016

hmm I see. maybe that might be sensible since horizontal scrolling is already working that way. then we also wouldn't have to worry about changing wheelHandler functionality in this PR.

@KamranAsif
Copy link
Contributor

LGTM, sorry for the delay; I was on vacation.
Do you need a new version?

@KamranAsif KamranAsif closed this Jan 9, 2017
@KamranAsif KamranAsif reopened this Jan 9, 2017
@KamranAsif KamranAsif merged commit 875fb9b into schrodinger:master Jan 9, 2017
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.

None yet

2 participants