Skip to content

Closes eligrey/classList.js#50#51

Open
moehlone wants to merge 4 commits into
eligrey:masterfrom
moehlone:master
Open

Closes eligrey/classList.js#50#51
moehlone wants to merge 4 commits into
eligrey:masterfrom
moehlone:master

Conversation

@moehlone

@moehlone moehlone commented Apr 4, 2016

Copy link
Copy Markdown

No description provided.

@moehlone

moehlone commented Apr 4, 2016

Copy link
Copy Markdown
Author

tested with IE8, IE9, Safari on iOS 6.1.3 (and all new browsers)

@eligrey

eligrey commented Apr 4, 2016

Copy link
Copy Markdown
Owner

Don't forget to change https://github.com/eligrey/classList.js/blob/master/classList.js#L3

Also, you're leaking a global (setPropOnDOM). You may need to use a closure.

@moehlone

moehlone commented Apr 4, 2016

Copy link
Copy Markdown
Author

Should be better now :)

Comment thread classList.js Outdated
/*
* classList.js: Cross-browser full element.classList implementation.
* 1.1.20150312
* 1.1.20150404

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.

Still living in 2015 I see. Mind updating that to 2016?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry.. fixed it

Comment thread classList.js
/*! @source http://purl.eligrey.com/github/classList.js/blob/master/classList.js */

if ("document" in self) {
(function() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The enclosure should probably enclose everything.

Suggest:

(function(self) {
  'use strict';

  if(!("document" in self)) {
    return;
  }

  // try/catch

  // ....

})(typeof self !== 'undefined' ? self : this);

This would help reduce nesting instead of increase it.

Such an enclosure would also fix: #48

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.

4 participants