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

inject whole head in header and footer #31

Closed
wants to merge 2 commits into from
Closed

inject whole head in header and footer #31

wants to merge 2 commits into from

Conversation

itajaja
Copy link

@itajaja itajaja commented Apr 26, 2015

The change allow the following imporvements:

  1. Use link elements defined in the head also into the header and footer
  2. Use multiple style tags. right now the selector would only take the first style tag and ignore the others

I am using it right now and I can't see any downside of this approach

inject all the head element into the header and footer
@marcbachmann
Copy link
Owner

Thanks for pointing out that I only used the first script tag.
Apparently that was good enough for me at the time of writing 😄

While your pull request is a good solution for most cases, it creates invalid html in the header & footer elements. A head element must have an html tag as parent.
It's easier to just copy all the link & script tags.
I'd rather use something like this:

styles = document.querySelectorAll('link,style')
styles = Array.prototype.reduce.call(styles, function(string, node){return string+node.outerHTML;},'')

@itajaja
Copy link
Author

itajaja commented Apr 26, 2015

Definitely a much better solution! Also, how do you build it? I don't see the instructions on the readme :)

@marcbachmann
Copy link
Owner

npm test also builds the coffeescript.
I've pushed v1.2.0 to github & npm

@itajaja
Copy link
Author

itajaja commented Apr 27, 2015

Great! let's close this PR then :)

@itajaja itajaja closed this Apr 27, 2015
@itajaja itajaja deleted the patch-1 branch April 27, 2015 07:58
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