search

jonkemp / gulp-inline-css

Inline linked css in an html file. Useful for emails.
MIT License
272 stars 29 forks source link

Read content from the stream, not from the filesystem #3

Closed jfache closed 10 years ago

jfache commented 10 years ago

Hi there,

I wrote a similar plugin before, and you fell into the same trap as I did - reading content from the filesystem versus from the stream.

As of now, if you modify the stream before passing it to gulp-inline-css (let's say, using gulp-replace), the modifications will be lost as gulp-inline-css reads its data from the filesystem (using file.path within its juice call).

Using juice.juiceContent() fixes that issue and actually deals with the stream content.

Let me know if my explanation is clear enough, thanks.

jonkemp commented 10 years ago

Thanks! Looks good to me, but the build is falling because the tests no longer pass. Any idea why?

jfache commented 10 years ago

Having a look right now.

jfache commented 10 years ago

Ok, found the issue with the test. The file.path needs to be absolute so that juice can properly link to the CSS file.

2 options:

  1. We use path.resolve in the test (test/main.js, line 12)
  2. We require path and use path.resolve in the plugin (index.js, line 15)

Option 2 adds a dependency that is useless in most cases, as gulp.src() always returns absolute path. I would go with option 1. What do you think?

jonkemp commented 10 years ago

Looks good. Not a big deal, but do you know how to squash commits? If you can do that, I would prefer it. Here's instructions on how to do it, if you need them. Thanks.

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

jfache commented 10 years ago

Commits squashed, ready to pull!