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

IE7 and IE8 - fixed! #1

Merged
merged 2 commits into from May 17, 2012

Conversation

@colinta
Copy link
Contributor

commented May 17, 2012

using $(this).text() does not return text in IE7 or IE8. Checking out iCanHaz, the code there was using innerHTML. Dropped that in here, and voila!

Also, some whitespace, so that JSHint would stop whining.

@bltavares

This comment has been minimized.

Copy link
Owner

commented May 17, 2012

Hi there @colinta ,

Thanks for sending the pull request. Does the selector need to be rewritten to work on IE?
I can test it by tomorrow on the office and then, merge it.

@colinta

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

It does not, apparently. I did change it (and it worked), but before sending the pull request I changed the query selectors back, and things still worked. Here's what I changed it to, a combination of document.getElementsByTagName('script') and $().filter()

$(document.getElementsByTagName('script')).filter(function() {
  var script = this;
  return script && script.innerHTML && script.id && (script.type === "text/html" || script.type === "text/x-_canhaz");
}).each(function(k,e) {
  var $this = $(this), text = $.trim(this.innerHTML);
  self.addTemplate($this.attr("id"), text);
  $this.remove();
});

But, like I said, it was only the innerHTML bit that seemed to be important.

@bltavares

This comment has been minimized.

Copy link
Owner

commented May 17, 2012

I see you like the text/x-_canhaz selector.

Do you mind testing this selector: 'script[type="text/html"],script[type="text/x-_canhaz"]' and instead of using innerHTML changing it to:
text = $.trim($this.html()); ?

I just tested on Chrome and Firefox and they are working. I can only test it on IE tomorrow.

If everything goes ok I will merge. (:

@colinta

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

aw geez, i just realized i had an unsaved change :-/ that last comment didn't make any sense...

you don't have to use that filter thing, i'll revert to $('script[type=text/html]'), since that part works fine.

(I grabbed that code from icanhaz, and just changed it to be consistent)

@colinta

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

OK, that's better. sorry 'bout the confusion!

@bltavares

This comment has been minimized.

Copy link
Owner

commented May 17, 2012

Don't worry. (:

Did the text = $.trim($this.html()); worked on IE?

@colinta

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

Nope, that's the part that didn't work. I called that $(this).text() above, I should have written $this.html(). The fix was to use this.innerHTML instead.

bltavares added a commit that referenced this pull request May 17, 2012

@bltavares bltavares merged commit 9e6ac9d into bltavares:master May 17, 2012

@bltavares

This comment has been minimized.

Copy link
Owner

commented May 17, 2012

Thank you @colinta (:
Glad for this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.