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

Fix broken flow through runValidations in IE8 #128

Merged
merged 1 commit into from May 20, 2016

Conversation

Projects
None yet
2 participants
@andymantell
Contributor

andymantell commented May 20, 2016

Hi,

I am aware that you don't officially support IE8, but I have got validate.js up and running perfectly by including various polyfills for the bits of missing JS functionality.

The only blocker to it running however was a test in the runValidations function which checks if attributes is a dom element (isDomElement). It was doing this by checking if querySelectorAll and querySelector had a type of function. The issue is that IE8 actually returns a type of 'object' for these native functions (See http://stackoverflow.com/questions/12933625/typeof-in-ie-8-for-native-functions). Therefore I have tweaked this test to merely check for the presence of these methods on the object and not to check on their type. I feel that it is reasonable to assume that if they are present, that they will be the native methods or something that functions equivalently (e.g. a polyfill) and that testing their type doesn't add any significant value.

What are your thoughts? As I say, this small tweak allows the library to run nicely under IE8 (provided you polyfill things like Array.prototype.forEach and a few others) without diminishing functionality in modern browsers.

Fix broken flow through runValidations in IE8
runValidations was failing in the isDomElement test due to testing if querySelector and querySelectorAll were functions, which fails in IE8
@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman May 20, 2016

Owner

This definitely sounds resonable, it that's all it takes besides the polyfills!

Owner

ansman commented May 20, 2016

This definitely sounds resonable, it that's all it takes besides the polyfills!

@ansman ansman merged commit d00b423 into ansman:master May 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman May 20, 2016

Owner

This is now released in 0.10.0

Owner

ansman commented May 20, 2016

This is now released in 0.10.0

@andymantell

This comment has been minimized.

Show comment
Hide comment
@andymantell

andymantell May 21, 2016

Contributor

Thanks @ansman, much appreciated

Contributor

andymantell commented May 21, 2016

Thanks @ansman, much appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment