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

Create only a single style element #1

Merged
merged 1 commit into from
Apr 4, 2016

Conversation

jantimon
Copy link
Contributor

I was interested about the progress of your plugin so I dived into your source.

Unfortunately this pull request changes several parts at once so feel free to pick only some of them.

  1. rimraf was missing so the tests failed
  2. I replaced this in addInlineCss as it was confusing for a prototype function to use a different this (hope now it's clear that it is the compilation).
  3. The injection now also works case insensitive e.g. </HEAD>.
  4. To save some bytes only one <style> tag is added, this might be better for projects with a lot of small css files.
  5. The tests now use an expected file.
  6. Fixed a typo in your test css file

@numical
Copy link
Owner

numical commented Mar 31, 2016

Thanks! Glad I left it a version 0.2.0!
As to your points:

  1. That'll teach me to test on Windows too
  2. Yes - much clearer, and cleaner than my comment
  3. Your RegEx foo is greater than mine
  4. Yes - thought people might want separate style tags for clarity. However this is much nicer for minification too (see later).
  5. I was trying to avoid too many fixtures as they become a real maintenance burden. Will probably stick with RegEx's.
  6. Ha! No! Not a typo - see test 'inlining works with postcss-loader' and use of 'postcss-spiffing' module - I use this to convert the British spelling to US one. However this certainly breaks the principle of least surprise - I will add a comment in the CSS to explain.

@numical
Copy link
Owner

numical commented Mar 31, 2016

Jan,
As I have your attention - I thought I would mention that I am not going to be asking for an additional event in html-webpack-plugin. This was going to be to separate the 'after-html-processing' from any minification stage so that this plugin could fit in between them and have its inlined CSS minified by the html-webpack-plugin's minifier.
However your current events are clean and consistent, and my request would have been a case of special pleading. Moreover one plugin's options affecting another is a bit messy. Hence I'm going to incorporate an optional call to clean-css within this plugin. Coming soon...

@jantimon
Copy link
Contributor Author

Cool :)
You should also try Travis - it's free and awesome for testing.. and quite easy.. it runs all your test for every commit and pull request automatically

@numical numical merged commit 30da299 into numical:master Apr 4, 2016
@jantimon
Copy link
Contributor Author

jantimon commented Apr 4, 2016

👍

numical pushed a commit that referenced this pull request Jan 9, 2018
added tests for public URL
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