8
\$\begingroup\$

This HTML code seems to work: the page looks as I intended it. I have checked it multiple times, but it's difficult to tell how to improve. I would like somebody to review and point out any mistakes or bad practices, or other recommendations to make it better.

This is to be output as an email:

 <!DOCTYPE: html><!-- variable:eHead -->
  <html>
  <title>Commentary Notification</title>
 <body style="margin: 0">
 <div align="center">
 <table cellpadding="0" cellspacing="0" width="600" bgcolor="#FFFDF2">
    <tr>
        <td style="padding: 5px 0px 5px 0px;" bgcolor="#FFFDF2">
            <tr>
                <td align="center"><a href=""><img alt="" src="" width="600" height="110"  style="display: block; font-family: Helvetica, Arial; color: #000000; font-size: 14px; padding: 0px 5px 0px 5px;" border="0"></a>

                </td>
            </tr>
            <tr>
                <td></td>
            </tr>
        </td>
    </tr>
    <!-- variable: eBody -->
    <tr>
        <td style="padding: 5px 0px 5px 0px;">
            <tr>
                <td bgcolor="#FFFDF2" colspan="2" style="padding: 0px 0px 0px 0px; font-size: 32px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #0A3775; text-shadow: 2px 2px 2px FFFDF2; text-align: center;">New Commentary Available</td>
            </tr>
            <tr>
                <td colspan="2" align="center" style="padding: 0px 45px 0px 45px; font-size: 16px; line-height: 22px; font-family: Helvetica, Arial, sans-serif; color: #062145;">A new commentary has been posted to the Commentary Blog.</td>
            </tr>
        </td>
        <!-- Variable: eBodyHead -->
        <table style="padding: 5px 0px 5px 0px;" cellpadding="0" cellspacing="0" width="600">
            <tr bgcolor="FFFDF2" style="padding: 15px 15px 15px 15px;">
                <table border="0" cellpadding="0" cellspacing="0" width="571">
                    <tr>
                        <td style="width:224px;" align="Left" style="padding: 0px 0px 10px 68px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Commentary:</td>
                        <td align="Left" style="width:327px; padding: 0px 0px 10px 20px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Current Item</td>
                    </tr>
                    <tr>
                        <td style="width:224px;" align="Left" style="padding: 0px 0px 10px 68px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Posted By:</td>
                        <td align="Left" style="width:327px; padding: 0px 0px 10px 20px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Created By</td>
                    </tr>
                    <tr>
                        <td style="width:224px;" align="Left" style="padding: 0px 0px 10px 68px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Category:</td>
                        <td align="Left" style="width:327px; padding: 0px 0px 10px 20px; font-size: 16px; font-family: Helvetica, Arial, sans-serif; font-weight: bold; color: #323232; letter-spacing:-1px; text-shadow: 1px 1px 1px #888888;">Cat</td>
                    </tr>
                    <tr>
                        <td colspan="2" align="center" style="padding: 0px 0px 0px 0px; font-size: 13px; line-height: 20px; font-family: Helvetica, Arial, sans-serif; color: #323232;">&nbsp;To access this Commentary post please click on the following link:</td>
                    </tr>
                    <tr>
                        <td>&nbsp;</td>
                    </tr>
                    <tr>
                        <td colspan="2" style="text-align: center; padding: 0px 0px 0px 10px"><a href="https://community.statestr.com/sites/SSgATradingDesk/SSgAMarketCommentaries" style="text-decoration: none; color: #D25C29;">Commentary Blog&rarr;</a>

                        </td>
                    </tr>
                    <tr>
                        <td>&nbsp;</td>
                    </tr>
                </table>
            </tr>
        </table>
        <!-- Variable: eFoot -->
        <table align="center" bgcolor="#E5E4D0" cellpadding="0" cellspacing="0" width="600" style="padding:0px 5px 0px 5px">
            <tr>
                <td>
                    <tr>
                        <td align="left" style="font-family: Helvetica, Arial, sans-serif; color: #162656; font-size: 12px; padding: 10px 15px 10px 15px; line-height: 18px; display: block;">This email and any files transmitted with it are confidential and intended solely for the use of the individual/s or entity to whom they are addressed. Please notify <span style="text-decoration: none" color="#888888">Email adress</span> immediately if you have received this e-mail by mistake and delete this e-mail from your system. If you are not the intended recipient you are notified that disclosing, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited.</td>
                    </tr>
                </td>
            </tr>
        </table>
 </div>
 </body>
 </html>
 </html>

\$\endgroup\$
2
  • 4
    \$\begingroup\$ Could you please clarify what you mean by "making it as accurate as possible"? \$\endgroup\$
    – syb0rg
    Commented Aug 15, 2014 at 21:23
  • \$\begingroup\$ Sorry that this was unclear - I would be grateful for any review that points out anything that is "broken" or illegal as the html is visually correct, but as pointed out below not necessarily efficient, readable or true to (somewhat unclear/hard to find) guidelines on using html in emails. I would like clean, (accurate to guidelines) code :) \$\endgroup\$
    – user3845582
    Commented Aug 16, 2014 at 23:34

2 Answers 2

Reset to default
13
\$\begingroup\$

Broken HTML

Browsers are forgiving. I see a fairly nice looking page, but the truth is, this HTML is seriously broken.

 <table cellpadding="0" cellspacing="0" width="600" bgcolor="#FFFDF2">
    <tr>
        <td style="padding: 5px 0px 5px 0px;" bgcolor="#FFFDF2">
            <tr>
                <td align="center"><a href=""><img alt="" src="" width="600" height="110"  style="display: block; font-family: Helvetica, Arial; color: #000000; font-size: 14px; padding: 0px 5px 0px 5px;" border="0"></a>

                </td>
            </tr>

You have table > tr > td > tr, which is illegal: you cannot have a tr element inside a td element.

Maybe you meant this structure instead:

<table cellpadding="0" cellspacing="0" width="600" bgcolor="#FFFDF2">
    <tr>
        <td style="padding: 5px 0px 5px 0px;" bgcolor="#FFFDF2">
            <table>
                <tr>
                    <td align="center"><a href=""><img alt="" src="" width="600" height="110"

Simple syntax errors

There are also several simple syntax errors:

  • The first line should be <!DOCTYPE html>, and NOT <!DOCTYPE: html>
  • You have this:
<td bgcolor="#FFFDF2" colspan="2"
    style="padding: 0px 0px 0px 0px; font-size: 32px;
           font-family: Helvetica, Arial, sans-serif;
           font-weight: bold; color: #0A3775;
           text-shadow: 2px 2px 2px FFFDF2; text-align: center;">
    New Commentary Available
</td>

The text-shadow color FFFDF2 should have been #FFFDF2.

Separate the CSS from HTML

The whole point of CSS is to separate the fine-tuned styling from the HTML. You really shouldn't inline so many style attributes everywhere. Move the content of all those inlined style declarations to a separate .css file. You will be able to reduce a lot of the duplication, and you will get a much more maintainable solution: you will be able to make style changes in one place and they will apply in all the places where that style should be used.

UPDATE: the original post did not mention that this HTML is to be used in an email, that bit was added later. And in that case, inline CSS can be acceptable:

http://css-tricks.com/using-css-in-html-emails-the-real-story/

(Thank you @pbk1303, excellent point & link!)

The ultimate test

There is a website that checks for correct HTML formatting and bad practices:

http://validator.w3.org/check

You can paste the content of your HTML file in the Validate by Direct Input tab and it will tell you what else is wrong with your HTML. Ideally you should only post on Code Review after you have corrected all the common violations captured by this tool.

Inlining CSS with npm juice

@Ben left an excellent remark in comments:

Might also be worth mentioning that you can install juice with npm, to be able to inline the CSS from an external file; that way you keep all the benefits of separate CSS whilst still being able to generate an email client friendly version. npmjs.org/package/juice

\$\endgroup\$
3
  • \$\begingroup\$ Thank you!! Very Much appreciated! Unfortunately some of the question/review description was edited away (mostly my poor grammar's fault). But this is to be output as an email. From my research I have gotten the impression that CSS is generally unsupported especially in outlook. therefore the inline CSS. Could someone confirm this? And thank you for the link, this is something that will really come in useful, will this allow inline or flag these as common violations? \$\endgroup\$
    – user3845582
    Commented Aug 16, 2014 at 23:04
  • 1
    \$\begingroup\$ The Validator will check for things like missing close tags and proper nesting. Inline Styles are perfectly valid, just a bad idea. (At least for an actual website, I can't speak to how it's best done for email.) \$\endgroup\$
    – RubberDuck
    Commented Aug 16, 2014 at 23:13
  • 3
    \$\begingroup\$ If you need to use this in an email, then embed the CSS in a <style type="text/css"></style> tag. That's perfectly fine, and much better than duplicating style= attributes in so many places. \$\endgroup\$
    – janos
    Commented Aug 16, 2014 at 23:38
5
\$\begingroup\$

A few other things I can see right off the bat:

  • you're missing a <head> block; it's supposed to be before the <body> block, and the <title> block is supposed to be inside it.
  • you've duplicated the ending </html>
  • what's the purpose of the HTML comments? They don't explain anything.
  • Is <a href=""> blank in the real HTML or was it just redacted for posting to CodeReview?
  • are you using &nbsp; for spacing purposes? If so, don't -- use the proper CSS attributes to control spacing.
\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.