Skip to content

Check eventListener 'options' support & use same passive arg for removal#131

Merged
caseywebdev merged 1 commit intocaseywebdev:masterfrom
STRML:passiveRemove
Aug 25, 2016
Merged

Check eventListener 'options' support & use same passive arg for removal#131
caseywebdev merged 1 commit intocaseywebdev:masterfrom
STRML:passiveRemove

Conversation

@STRML
Copy link
Copy Markdown
Contributor

@STRML STRML commented Aug 25, 2016

The DOM Spec indicates that options should be identical when removing event listeners.

We also should check for event listener options support before using passive, lest we accidentally set capture by passing a truthy value and get different behavior on older browsers.

Comment thread react-list.es6 Outdated
}

componentDidMount() {
this.PASSIVE = supportsPassive() ? {passive: true} : false;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can't this just be a constant in the module scope?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I was thinking it would obviously crash on the server but the
try/catch or a process.browser check will clear that right up.

On Aug 25, 2016 11:36 AM, "Casey Foster" notifications@github.com wrote:

In react-list.es6
#131 (comment):

@@ -72,6 +86,7 @@ module.exports = class ReactList extends Component {
}

componentDidMount() {

  • this.PASSIVE = supportsPassive() ? {passive: true} : false;

Can't this just be a constant in the module scope?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/orgsync/react-list/pull/131/files/7cc25b30658d7f879ac100b766896cd857b12e21#r76278814,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABJFP50txIkKvSqJdKZXzb5Fvfpb3vyhks5qjcR1gaJpZM4JtOs9
.

@STRML
Copy link
Copy Markdown
Contributor Author

STRML commented Aug 25, 2016

Updated @caseywebdev

Comment thread react-list.es6 Outdated
// add/removeEventListener, we need to check, otherwise we will
// accidentally set `capture` with a truthy value.
const PASSIVE = (() => {
if (!process.browser) return false;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not familiar with process.browser? AFAIK process isn't a standard global in the browser. Can we go with typeof window === 'undefined'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I apologize - I write so many webpack/browserify modules that I just assume those globals. Will fix.

@STRML
Copy link
Copy Markdown
Contributor Author

STRML commented Aug 25, 2016

Updated

@caseywebdev
Copy link
Copy Markdown
Owner

Cool, LGTM, thanks!

@caseywebdev caseywebdev merged commit 8b8c22f into caseywebdev:master Aug 25, 2016
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.

2 participants