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 passive option to touch event listeners. #680

Merged
merged 3 commits into from
Jan 5, 2017
Merged

Add passive option to touch event listeners. #680

merged 3 commits into from
Jan 5, 2017

Conversation

crhallberg
Copy link
Contributor

@crhallberg crhallberg commented Jan 3, 2017

By adding a passive option to the touchstart and touchmove events we can let the browser know that we do not intend to ever cancel scroll events. I found no such cases in _touchstart nor _touchmove. This will prevent any scroll lag that may be caused by this library.

More on passive event listeners

  • unit tests (we use Jasmine 1.3)
  • JSFiddle (no visible new features)
  • documentation updates to README file (N/A)
  • examples within /tpl/index.tpl (for new options being added) (N/A)
  • Passes CI-server checks (runs automated tests, JS source-code linting, etc..). To run these on your local machine, type grunt test in your Terminal within the bootstrap-slider repository directory

There is also some trailing whitespace cleanup I can remove if you'd prefer.

@seiyria
Copy link
Owner

seiyria commented Jan 3, 2017

I like this, but:

  • please do not rebuild dist
  • I think we should have a fallback for older browser (see here), since an object is technically truthy, but the object format is not always supported.
  • it's gotta pass travis

@crhallberg
Copy link
Contributor Author

Understood.

Is this a usual warning to get: WARNING: $.fn.slider namespace is already bound. Use the $.fn.bootstrapSlider namespace instead.

@seiyria
Copy link
Owner

seiyria commented Jan 3, 2017

If you have jquery UI slider, then I can see that happening. Otherwise, no.

@crhallberg
Copy link
Contributor Author

crhallberg commented Jan 3, 2017

This is happening when I run the grunt test. Even on Travis. Does this look better? I found a way to fallback using a quick functionality check.

@seiyria
Copy link
Owner

seiyria commented Jan 3, 2017

I don't actually know how effectively that would work, to be perfectly honest.

@crhallberg
Copy link
Contributor Author

The detection is the same used by a few libraries (like detect-it.js) so I have confidence in it,

@@ -741,9 +741,21 @@ const windowIsDefined = (typeof window === "object");
this.touchmove = this._touchmove.bind(this);

if (this.touchCapable) {
// Test for passive event support
var supportsPassive = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you declare this variable with let instead of var to scope to nearest enclosing block?

// Test for passive event support
var supportsPassive = false;
try {
var opts = Object.defineProperty({}, 'passive', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you declare opts with let instead of var to ensure it is only scoped within the try {} block?

@rovolution
Copy link
Collaborator

Great PR @crhallberg! In regards to the fallback check, I saw that Paul Irish himself recommends a similar technique, so I am cool with that.

I left some feedback in the code so if you could incorporate that and ping me that would be much appreciated. Thanks!

@crhallberg
Copy link
Contributor Author

I resolved the conflict with the date change and converted my variables to let declarations.

@rovolution rovolution merged commit e581dd1 into seiyria:master Jan 5, 2017
@rovolution
Copy link
Collaborator

Merged and published to v9.7.0. Thanks again for your contribution @crhallberg!

@crhallberg
Copy link
Contributor Author

Thanks for letting me!

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