forwardemail / email-templates

Create, preview (browser/iOS Simulator), and send custom email templates for Node.js. Made for @forwardemail, @ladjs, @cabinjs, @spamscanner, and @breejs.
https://forwardemail.net/docs/send-emails-with-node-js-javascript
MIT License
3.67k stars 337 forks source link

Preferred text.pug file over html2text #282

Closed mrlannigan closed 6 years ago

mrlannigan commented 6 years ago

I have a use case where I need html2text to run when a text template file is missing, but I need the text template to be used if it does exist.

With this change, I have preferred the text template file over the result of html2text. I have also added a bit more coverage on the contents of the message returned from the send method.

kibertoad commented 6 years ago

@mrlannigan I presume this needs documentation?..

codecov-io commented 6 years ago

Codecov Report

Merging #282 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #282   +/-   ##
=======================================
  Coverage   90.35%   90.35%           
=======================================
  Files           1        1           
  Lines         114      114           
=======================================
  Hits          103      103           
  Misses         11       11
Impacted Files Coverage Δ
src/index.js 90.35% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 697c7e2...5e06d97. Read the comment docs.

kibertoad commented 6 years ago

@mrlannigan Also coverage is down, so it looks like some of the new code lacks tests.

niftylettuce commented 6 years ago

Yeah if you can document this in README that'd be awesome.

mrlannigan commented 6 years ago

@kibertoad I'll work on getting the coverage up.

@niftylettuce Do you have a suggested place in the README where I should document this change. There doesn't seem to really be a place now where it's explained. Maybe I could add a blurb under Options header?

niftylettuce commented 6 years ago

Yeah probably in options section, and maybe also then just add another example section called like "Selectively Render HTML" or something. Your call for naming whatever makes sense is cool, options is good enough but if you wanted to document further up to you

On Feb 11, 2018 3:34 PM, "Julian Lannigan" notifications@github.com wrote:

@kibertoad https://github.com/kibertoad I'll work on getting the coverage up.

@niftylettuce https://github.com/niftylettuce Do you have a suggested place in the README where I should document this change. There doesn't seem to really be a place now where it's explained. Maybe I could add a blurb under Options header?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/niftylettuce/email-templates/pull/282#issuecomment-364785892, or mute the thread https://github.com/notifications/unsubscribe-auth/AAf7hd5-p9pntiQJk8p2F23tqit7V5p7ks5tT07AgaJpZM4SBett .

mrlannigan commented 6 years ago

@kibertoad + @niftylettuce I've updated the README.

kibertoad commented 6 years ago

@niftylettuce Probably could be merged?..

niftylettuce commented 6 years ago

This has been released in v3.4.0 now available on npm.

npm install email-templates@latest or yarn add email-templates@latest

Thanks for your contribution