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

IE8 Compatibility #103

Closed
wants to merge 2 commits into from
Closed

IE8 Compatibility #103

wants to merge 2 commits into from

Conversation

sorrycc
Copy link

@sorrycc sorrycc commented Aug 19, 2015

Close #102


for (var i=0; i<dotCount; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@sorrycc I don't see a need for this change. Please revert changes on this particular file.

I'll look at this PR in the weekend. I am bit busy with my college work.

Copy link
Author

Choose a reason for hiding this comment

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

It's don't work in IE8 either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assume one has included es5-shim for IE8 supporting, .map for array should be safe. The problem is within .apply. It should be: var dots = Array.apply(null, Array(dotCount + 1).join('0').split('')).map(...
Tested with native IE8, it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nosir Thanks for your comment. My knowledge about ie8 is next to nothing. @sorrycc Is es5-shim an acceptable solution in your case? I really don't want to keep any for loops in my code unless it is required seriously.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vramana To make ReactJS work with IE8, you have to include es5-shim anyway. Also I think it's not necessary to include the polyfill for addEventListener, it should be specified in README like ReactJS does in its doc.
http://facebook.github.io/react/docs/working-with-the-browser.html#browser-support-and-polyfills
I can send another PR for all these stuffs

Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to: #110
Thanks to @sorrycc for targeting the issue.

@vramana vramana closed this Sep 11, 2015
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.

None yet

3 participants