inliner() crashes on pages in subfolders of /pages/

cossssmin commented 8 years ago

How can we reproduce this bug?

Create a blueprints folder under src/pages/, and add a HTML file that contains Inky markup, or imports partials - doesn't matter. Mine imports partials, I called it my-blueprint.html.

What did you expect to happen?

When running npm run build, I expected that everything builds for production and the dist folder contains a blueprints/my-blueprint.html with inlined CSS.

What happened instead?

blueprints/my-blueprint.html is created by pages(), no problem. But the inliner() errors out when it reaches this file and tries to inline. This is what I get:

[18:07:55] Starting 'inline'...
Unhandled rejection Error: ENOENT: no such file or directory, open 'C:\DEV\my-template\dist\blueprints\css\app.css'
    at Error (native)
[18:07:57] The following tasks did not complete: default, build, inline
[18:07:57] Did you forget to signal async completion?

So my question is: how can one create subfolders in src/pages/ for better control of their project structure, and still use npm run build?

I suppose it has to do with the dist/css/app.css path being treated as 'relative' by the inliner(), but I have no idea how to make it work.

cossssmin commented 8 years ago

Took one more look, it seems it only has to do with the way the CSS file is read just before being passed to siphon.

It's an issue with fs.readFileSync: the inliner() is called recursively on every HTML file in the dist folder, and receives a relative path to the CSS file that is to be parsed.

But whatever I try, even defining a constant like const CSSFILE = path.resolve(__dirname, 'dist/css/app.css'); and providing that to the inline() function, it still doesn't work. __dirname, process.cwd() - nothing works...

Does anybody know how the fs.readFileSync function could read the same file?

cossssmin commented 8 years ago

Scratch the above, the issue is with the <link> tag in the layout file, and I've managed to fix it.

To help others, here's a step-by-step guide :

Step 1

Declare a constant for the compiled CSS path. After the PRODUCTION constant in gulpfile.babel.js, I added:

// Absolute path to the compiled CSS
const CSSFILE = 'file://' + path.join(__dirname, 'dist/css/app.css');

Step 2

In my default.html layout, I changed:

<link rel="stylesheet" type="text/css" href="css/app.css">

to

<link rel="stylesheet" type="text/css" href="$CSSFILE$">

Step 3

In the inliner() function, right before this pipe:

.pipe($.inlineCss, {

I added this one, too:

.pipe($.replace, '$CSSFILE$', CSSFILE)

Step 4

Finally, you need to update the line that removes the link tag from the HTML, once everything is done:

.pipe($.replace, '<link rel="stylesheet" type="text/css" href="css/app.css">', '')

becomes:

.pipe($.replace, '<link rel="stylesheet" type="text/css" href="'+CSSFILE+'">', '')

P.S.: if this looks good to you, I can do a PR.

cossssmin commented 8 years ago

Searching the gulp-inline-css repo for issues, I found a much cleaner way - https://github.com/jonkemp/gulp-inline-css/issues/36#issuecomment-228594209

Discard the above comment, I've tested this and it works:

In layouts/default.html, add a / before the CSS file path:

<link rel="stylesheet" type="text/css" href="/css/app.css">

Add a url option to the inlineCSS pipe:

.pipe($.inlineCss, {
      url: 'file://' + __dirname + '/dist/',
      applyStyleTags: false,
      removeStyleTags: false,
      removeLinkTags: false
    })

Hope this helps :)

HansUXdev commented 8 years ago

Is the inline task even needed now that gmail supports style tags?

cossssmin commented 8 years ago

So when the gradual release for responsive support in Gmail is done, we get all green for major browsers that support embedded CSS here.

However, there are still other email clients that won't support it, so it's not yet fully safe to stop inlining. Depends on your audience, but with a framework you can't just drop all.

I am thinking maybe the npm start tasks could be modified so that, instead of leaving the CSS file untouched in the <head>, it could pass it through siphon-media-query and then remove the <link> tag. All while NOT inlining. Or, the above functionality could be added as a flag to the build process, i.e. npm run build --embedded.

I am very curious about this and will run some tests with Email on Acid over the weekend - I have a project just in time for it.

Dropping inlining would be HUGE if client support would be good enough. Not only because of the clean files that would result, but also because of much smaller file sizes. Which means faster loading emails, and less likely to hit the 102KB limit that Gmail has.

I've also started a discussion on the forum about this.

foundation / foundation-emails