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

Fixing Firefox and IE8 compatibility issues #75

Merged
merged 19 commits into from
Jan 23, 2014

Conversation

thom4parisot
Copy link
Contributor

We now have a fully covered unit testing for:

  • Safari 6 (OS X)
  • Mobile Safari (iOS 5.1)
  • Android Stock Browser (Android 4.0)
  • Firefox 21 (Windows 7)
  • Internet Explorer 8 (Windows 7)

This is the result of the following PR:

They have been merged in the browsers-compatibility branch first.
Secured data are not pushed in PR incoming from forks.

If the Travis badge is failing for this PR, consider this build as a reference:

https://travis-ci.org/BBC-News/Imager.js/builds/17475266

thom4parisot and others added 18 commits January 21, 2014 14:42
`hasOwnProperty('length')` is not supposed to work on an `NodeList` as it's an inherited property.
It was not overridden in Gecko. The code was altered to accept an optional context, possibly not containing the expected property.
It is compatible with Mocha, Karma and mostly, IE6!
It then requires a very few changes from chai.
[READY] IE8 Compatible Unit Testing
Thus switching from `getElementsByClassName` to `getElementsByTagName` and targeting only divs.
[READY] Internet Explorer 8 Features Compatibility
@Integralist
Copy link
Contributor

@oncletom just a couple of quick observations...

    addEvent = (function(){
        if (document.addEventListener){
            return function addStandartEventLister(el, eventName, fn){
                return el.addEventListener(eventName, fn, false);
            };
        }
        else {
            return function addIEEventListener(el, eventName, fn){
                return el.attachEvent('on'+eventName, fn);
            };
        }
    })();

addStandartEventLister should probably be addStandardEventListener

Also, later on when you use the addEvent method you do...

addEvent(window, 'resize', function(){
    self.checkImagesNeedReplacing(self.divs);
}, false);

...you can see you pass in false but you use that by default within addStandartEventLister any way, so I'd assume we can lose the 4th argument false from the addEvent call.

@Integralist
Copy link
Contributor

@oncletom otherwise it looks good.

@thom4parisot
Copy link
Contributor Author

@Integralist oh well spotted, true. Fixed, thanks :-)

Integralist added a commit that referenced this pull request Jan 23, 2014
@Integralist Integralist merged commit cd4b1c5 into master Jan 23, 2014
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