Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
367 stars 66 forks source link

Suggested previewing technique doesn't actually inline the CSS #24

Closed jonleighton closed 9 years ago

jonleighton commented 9 years ago

Thanks for Roadie!

I followed the instructions at https://github.com/Mange/roadie-rails#previewing. However in order to actually get the CSS inlined when previewing (to ensure that it's working correctly), I had to add:

Roadie::Rails::MailInliner.new(email, email.roadie_options).execute

This seems to be because Roadie hooks in when calling #deliver, which doesn't happen when previewing.

Mange commented 9 years ago

Ah, yes. This is because you are using the Automatic inliner, which only inlines on delivery. I should update the README to make this clearer.

Automatic turned out to be more problematic (and more widely used than the "real" inliner) than I expected. I should also add a method to it for manual inlining, just for cases like this.

In your case, would a new config option toggling this "on delivery" behavior be preferred, or a simple method on a mail from Automatic that just inlines immediately (think mail.roadie_transform)?

ons 26 nov 2014 20:08 Jon Leighton notifications@github.com skrev:

Thanks for Roadie!

I followed the instructions at https://github.com/Mange/roadie-rails#previewing. However in order to actually get the CSS inlined when previewing (to ensure that it's working correctly), I had to add:

Roadie::Rails::MailInliner.new(email, email.roadie_options).execute

This seems to be because Roadie hooks in when calling #deliver, which doesn't happen when previewing.

— Reply to this email directly or view it on GitHub https://github.com/Mange/roadie-rails/issues/24.

jonleighton commented 9 years ago

I think the "only inline on delivery" behaviour is actually a bit confusing. May I suggest that you could do something like this:

module Roadie::Rails::Mailer
  def mail(options = {}, &block)
    if options.delete(:inline_css)
      MailInliner.new(super, roadie_options).execute
    else
      super
    end
  end

  def roadie_mail(options = {}, &block)
    mail(options.merge(inline_css: true), &block)
  end
end

module Roadie::Rails::Automatic
  def self.included(klass)
    klass.send :include, Roadie::Rails::Mailer
  end

  def mail(options = {}, &block)
    super(options.merge(inline_css: options.fetch(:inline_css, true)), &block)
  end
end

So basically this would make the inlining controlled purely by the :inline_css option, and the "automatic" module would just serve to set that option to true by default.

Mange commented 9 years ago

I like your style. I agree, in hindsight, that the way Automatic was built was a huge mistake. Since I'm doing semantic versioning I cannot change this behavior without bumping a major version, so I'll hold off from doing it a while longer. I might add a third module that behaves in a simpler way in the mean time. It will most likely work close to the way you describe here.

Mange commented 9 years ago

I opened issue #28 for this particular change. Please direct further comments about the future there.

I'll close this issue now as I think it is resolved. If not, feel free to reopen it again.