-
Notifications
You must be signed in to change notification settings - Fork 457
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
web: make it possible to send HTML email #3058
Conversation
html/inc/email.inc
Outdated
} | ||
if ($body_html) { | ||
$body = "<html>\n"; | ||
$body .= '<body style="font-family:Verdana, Verdana, Geneva, sans-serif; font-size:12px;">\n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to leave off the style tag on the body element? If a project is using HTML emails they are probably trying to replicate the styles they are using on their website and things like font and size may be something they want to control.
html/inc/email.inc
Outdated
$body = $body_html; | ||
$body .= "</body>\n"; | ||
$body .= "</html>\n"; | ||
$headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the charset iso-8859-1, if a user has registered a username with Chinese characters, how will this be rendered on the email client?
I changed it to UTF-8 |
html/inc/email.inc
Outdated
} | ||
if ($body_html) { | ||
$body = "<html><body>\n"; | ||
$body = $body_html; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to append to the string on this line. At the moment this overwrites from the previous line.
Sorry for the delay in getting back to this. I found a small bug that needs fixing otherwise it should be good to merge. |
Fixed; thanks! |
Codecov Report
@@ Coverage Diff @@
## master #3058 +/- ##
=========================================
Coverage ? 10.13%
=========================================
Files ? 39
Lines ? 6266
Branches ? 0
=========================================
Hits ? 635
Misses ? 5631
Partials ? 0 Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #3058 +/- ##
=========================================
Coverage ? 10.13%
=========================================
Files ? 39
Lines ? 6266
Branches ? 0
=========================================
Hits ? 635
Misses ? 5631
Partials ? 0 Continue to review full report at Codecov.
|
Looks fine - merging. Thanks for the change. |
No description provided.