JoshBarr / on-media-query

A neat way to trigger JS when media queries change. No jQuery required.
283 stars 33 forks source link

fix an error of varible declaration and an error in older versions of IE #18

Closed chenzhenxi closed 11 years ago

chenzhenxi commented 11 years ago

Error of varible declaration : There is a variable declared but never used in function mq.listenForChange. I think is maybe a error, I change variable name to query_string.

Error in older versions of IE MQ.removeQuery threw a null error in older version of IE because old IE not support Array.properties.indexOf. more info here: http://kangax.github.io/es5-compat-table/#Array.prototype.indexOf . the fix does'nt seem to throw a new error in other browsers.

mhulse commented 11 years ago

6f86d31 looks like a simple/good fix to me.

The other, 8ce84df, I can't tell just by looking. It sounds/looks like a good fix, but I'd have to test to be certain.

Just for the second pair of eyeballs, I'd like to test this for myself.

@chenzhenxi what versions of old IE were throwing the null error?

mhulse commented 11 years ago

@chenzhenxi what versions of old IE were throwing the null error?

Ah, I see that the link you posted shows that IE8 is the culprit?

I'm surprised that we have not seen this bug before (I have to admit that's one part of this code I rarely use, so I guess I can see why something like this would be missed)?

Do you have a demo page that I could look at? Does this happen on this repo's demo page?

Thanks! Micky

chenzhenxi commented 11 years ago

I added some test code at demo page (index.html). I've tested this in the latest releases of Chrome & Firefox, as well as IE 8 & 9 & 10. They didn't throw any javascript errors. It is always nice to test more. :)

mhulse commented 11 years ago

Awesome, thanks @chenzhenxi! I've got a subscription to browserstack.com, so I'll run it through the paces there as well.

I'll be back with my results. Thanks again for your contribution. Hopefully we'll get this code pulled in soon with the approval of the other guys in charge around here. :+1:

chenzhenxi commented 11 years ago

@mhulse my pleasure. ^ω^

patocallaghan commented 11 years ago

Thanks @chenzhenxi! Really surprised this was never spotted before...

@mhulse if you're happy with the fix let me know and I'll pull it in...

mhulse commented 11 years ago

@patocallaghan It looks good to me! :)

Because I have OCD (Obsessive Coding Disorder), I just wanted to do some browser testing before merging (was going to try getting to that today). But, if you think it looks good, I'd say merge 'er in. :+1:

Thanks!

mhulse commented 11 years ago

Doing tests now. I'll be back shortly with an update.

mhulse commented 11 years ago

Quick tests of old IEs (back to v6) and various desktop browsers/mobile devices look good on my end. I'm satisfied. :+1:

A few minor notes: console is not available in old IEs. I typically add var console = window.console || { log : function() {}, warn : function() {} }; but it was not working this time for some reason. Rather than trying to get console to work (in conjunction with the debug tools available to IE in www.browserstack.com), I kept my backup for the console and just added alert statements.

patocallaghan commented 11 years ago

Thanks guys!

chenzhenxi commented 11 years ago

:+1: yeah~

BTW, I am new to New Zealand. And I am looking for a job as programmer~ :) I'm good at HTML5, Java and a little C#. Actually, I sent my CV to hello@springload.co.nz ,few days ago. My CV include a lot of my works.

@JoshBarr ,Could I have a chance to have a interview? @patocallaghan , @mhulse Do you have some NZ chance for me?

No pay volunteer and intern is Okay for me. I really need gain some local work experience.

My email is zhenxi.chen@gmail.com. If your guys want my CV I can send to you.

Thx. :)

mhulse commented 11 years ago

Woot! Teamwork! :)

Got to love GitHub.

@chenzhenxi unfortunately, I'm just work as web monkey at a newspaper company, so I don't have any good work to offer you.

Places that I like to communicate with my peers in the community are (sorry in advance if you are already aware of all these things):

Anyway, not sure if that's of any help. Just a few things I could think of, that have helped/worked for me, off the top of my head.

Thanks again for your contribution!

Cheers, Micky

chenzhenxi commented 11 years ago

@mhulse Really helpful. :+1: I will try my best to get in to the community. :)