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

Support passive event #44

Merged
merged 13 commits into from
Dec 20, 2017
Merged

Support passive event #44

merged 13 commits into from
Dec 20, 2017

Conversation

roderickhsiao
Copy link
Contributor

@roderickhsiao roderickhsiao commented Dec 11, 2017

The follow up PR based on #43 to address #42

passing an option to the event listener instead of specifically passing passive: true so that we can support other event.

The event object will only be passed when browser support the event object.

@hankhsiao @redonkulus @kaesonho

Note: This PR will turn passive event default to true.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 59ecfd9 on roderickhsiao:passive into fc1245f on yahoo:master.

@@ -5,12 +5,14 @@
'use strict';

var EventEmitter = require('eventemitter3');
var supoortPassiveEvent = require('./lib/supportPassiveEvent');
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo extra o

Copy link
Collaborator

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

@roderickhsiao some minor comments. Any way to reliable test this?

@gyehuda can you give me admin access to this repo?

var add = 'addEventListener';
var remove = 'removeEventListener';
var eventOptions = globalVars.supportsPassive ? _assign(defaultEventOption, options) : false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, but maybe add {} as first argument to _assign to avoid changing defaultEventOption.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 49abc99 on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 49abc99 on roderickhsiao:passive into fc1245f on yahoo:master.

@roderickhsiao
Copy link
Contributor Author

@redonkulus good question about the testing, probably in functional test, but not sure if it is reliable though

@gyehuda
Copy link

gyehuda commented Dec 18, 2017

@redonkulus done.

@roderickhsiao
Copy link
Contributor Author

@redonkulus the below unit test could be added

5203eee

after JSDOM support passive event
https://github.com/tmpvar/jsdom/blob/master/test/web-platform-tests/to-run.yaml#L132
jsdom/jsdom#1609

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 5203eee on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 535c316 on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling db06337 on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 8bc9034 on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling eaec9fc on roderickhsiao:passive into fc1245f on yahoo:master.

@roderickhsiao
Copy link
Contributor Author

I tried to add test in functional test, but unfortunately I think functional test doesn't run for a long time, and its expecting @hankhsiao access token :P

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 54170f1 on roderickhsiao:passive into fc1245f on yahoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.76% when pulling 54170f1 on roderickhsiao:passive into fc1245f on yahoo:master.

@hankhsiao
Copy link
Contributor

It looks good to me 👍

@redonkulus redonkulus merged commit 7bd06b1 into yahoo:master Dec 20, 2017
@redonkulus
Copy link
Collaborator

Thanks!

This was referenced Dec 20, 2017
@roderickhsiao
Copy link
Contributor Author

🎅 🤶

@roderickhsiao roderickhsiao deleted the passive branch December 20, 2017 23:36
@redonkulus
Copy link
Collaborator

released in 1.1.0 🎄

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

5 participants