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

Closes eligrey/classList.js#50 #51

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

moehlone
Copy link

@moehlone moehlone commented Apr 4, 2016

No description provided.

@moehlone
Copy link
Author

moehlone commented Apr 4, 2016

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

@eligrey
Copy link
Owner

eligrey commented Apr 4, 2016

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
Copy link
Author

moehlone commented Apr 4, 2016

Should be better now :)

@@ -1,6 +1,6 @@
/*
* classList.js: Cross-browser full element.classList implementation.
* 1.1.20150312
* 1.1.20150404
Copy link
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
Author

Choose a reason for hiding this comment

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

Sorry.. fixed it

@@ -12,229 +12,244 @@
/*! @source http://purl.eligrey.com/github/classList.js/blob/master/classList.js */

if ("document" in self) {
(function() {
Copy link
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