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

Suggestion: improvement for 'Margin' #443

Open
davidhouweling opened this issue May 17, 2016 · 3 comments
Open

Suggestion: improvement for 'Margin' #443

davidhouweling opened this issue May 17, 2016 · 3 comments

Comments

@davidhouweling
Copy link

davidhouweling commented May 17, 2016

Whilst it isn't really an issue, I find it really bad having to duplicate all margin attributes with a captial M version (and it also breaks sass-linter).

I suggest updating the inliner function to take care of this instead.

This is what I currently use to work with the existing foundation-emails 2.1 codebase (starting the line after the mqCss injection).

function inliner(css) {
  var css = fs.readFileSync(css).toString();
  var mqCss = siphon(css);

  var pipe = lazypipe()
    .pipe($.inlineCss, {
      applyStyleTags: false,
      removeStyleTags: false,
      removeLinkTags: false
    })
    .pipe($.replace, '<!-- <style> -->', `<style>${mqCss}</style>`)
    // ADDED: so we don't have to do the Outlook-only (capital 'M') margin stuff, lets do it here...
    // 1. strip out the ones injected in by foundation-emails
    .pipe($.replace, /M(argin(?:-(?:top|bottom|left|right))?):([^";}]+;?)/g, '')
    // 2. duplicate all margin* attributes with their Margin* equals
    .pipe(
      $.replace,
      /m(argin(?:-(?:top|bottom|left|right))?):([^";}]+)((?:;(?!M))|"|})/g,
      'm$1:$2;M$1:$2$3'
    )
    .pipe($.htmlmin, {
      collapseWhitespace: true,
      minifyCSS: true
    });

  return pipe();
}

However, if we do clean out foundation-emails, then it could be simplified to...

function inliner(css) {
  var css = fs.readFileSync(css).toString();
  var mqCss = siphon(css);

  var pipe = lazypipe()
    .pipe($.inlineCss, {
      applyStyleTags: false,
      removeStyleTags: false,
      removeLinkTags: false
    })
    .pipe($.replace, '<!-- <style> -->', `<style>${mqCss}</style>`)
    // so we don't have to do the Outlook-only (capital 'M') margin stuff, lets do it here...
    .pipe(
      $.replace,
      /m(argin(?:-(?:top|bottom|left|right))?):([^";}]+)((?:;(?!M))|"|})/g,
      'm$1:$2;M$1:$2$3'
    )
    .pipe($.htmlmin, {
      collapseWhitespace: true,
      minifyCSS: true
    });

  return pipe();
}

What i am not sure of is if there are any specific Outlook-only margin rules that are only applied with Margin* and not margin*.

@rafibomb
Copy link
Member

rafibomb commented May 17, 2016

This is a really great idea and solution would work. Sometimes we're afraid of abstracting too much from the end user, but this one seems harmless. Using both Margin and margin have not caused issues anywhere in testing :)

There might be a better place to add this code: Create a Gulp task, like inliner-margin.js or something that gets imported in the gulpfile.

@davidhouweling
Copy link
Author

I'm not sure if it is necessary to have it separate as it is really just specific to the inlining work. When you do local development those rules don't have any impact, so it is really just needed during the inlining process (and hence made sense to add it there). Also if it was a separate task, it would then require further piping which may make it more confusing than just having it as a sort of 'step by step' to the inliner.

@rafibomb
Copy link
Member

In terms of making this happen, the best way would be getting this added to the inliner being used here https://github.com/jonkemp/inline-css

The issue can be opened there with the description that this would help email compatibility. Perhaps a Pull Request would go further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants