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

div injected inside head tag, and whitespace issues on generated css #144

Closed
toddmotto opened this issue Oct 30, 2013 · 7 comments
Closed

Comments

@toddmotto
Copy link

As per screenshot, a div is injected into the head element, which is wrong. I'd suggest opting for an attribute/attrs on <style> such as <style id="fitvids">.

screen shot 2013-10-30 at 21 12 26

Once rendered in the DOM, you've got a lot of whitespace, a simple /\s/g replace before appending would solve this for anyone debugging/viewing the code in Dev Tools:

screen shot 2013-10-30 at 21 17 48

@davatron5000
Copy link
Owner

  1. The whitespace issue should have been addressed. Are you using the latest version?
  2. The div injection is necessary due to a scope issue in IE

@toddmotto
Copy link
Author

Just inspecting element on fitvidsjs.com. And that's interesting, what's the scope issue w/IE?

@toddmotto
Copy link
Author

Ah, that'll teach me for reading messages without looking at the hyperlink ;) haha... Have you looked at either:

IE:
style.styleSheet.cssText = css;

All others:
document.createTextNode(css)

@davatron5000
Copy link
Owner

It hasn't come up, because what we have is working. I suppose it's possible (??). It would just require a pretty big round of testing to make sure.

@toddmotto
Copy link
Author

Yeah, it would definitely be an improvement, injecting into the DOM as a string isn't always perfect. I don't know if you've seen FluidVids, but it's a raw JS equivalent, which makes use of this technique (just pushed an update): https://github.com/toddmotto/fluidvids/blob/master/dist/fluidvids.js. Feel free to integrate into FitVids the few lines of code.

@davatron5000
Copy link
Owner

Nice! Might do. Let me know if run into any problems. 


Sent from Mailbox for iPhone

On Wed, Oct 30, 2013 at 5:21 PM, Todd Motto [email protected]
wrote:

Yeah, it would definitely be an improvement, injecting into the DOM as a string isn't always perfect. I don't know if you've seen FluidVids, but it's a raw JS equivalent, which makes use of this technique (just pushed an update): https://github.com/toddmotto/fluidvids/blob/master/dist/fluidvids.js. Feel free to integrate into FitVids the few lines of code.

Reply to this email directly or view it on GitHub:
#144 (comment)

@davatron5000
Copy link
Owner

Merging this into #159

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

No branches or pull requests

2 participants