Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm on a project involving converting HTML code to HTML5, and I'd like some feedback as to whether my code is correct or not.

Here's my stripped-down code:

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Page Title Goes Here</title>
<!--[if IE 9]>
<script src="/jsp/fileFolderPath/js/html5shiv.js"></script>
<![endif]-->

<link rel="stylesheet" href="/jsp/fileFolderPath/css/global.css">

<!--[if IE 7]> <html class="ie7" lang="en"> <![endif]-->
<!--[if IE 8]> <html class="ie8" lang="en"> <![endif]-->

<script src="/jsp/fileFolderPath/js/jquery-1.4.2.min.js"></script>
<script src="/jsp/fileFolderPath/js/jquery.colorbox-min.js"></script>
<script src="/jsp/fileFolderPath/js/tip.js"></script>
<script src="/jsp/fileFolderPath/js/jquery.calendar.js"></script>

<script src="/jsp/fileFolderPath/js/modernizer-2.0.6.min.js"></script>

<script type="text/javascript">
$(document).ready(function() {
    $('#whatsthis').mouseover(function(){
        tooltip.show('Tooltip text goes here');
    }).mouseout(function(){
        tooltip.hide();
    });

    $('#printthis').mouseover(function(){
        tooltip.show('&lt;strong&gt;Print&lt;/strong&gt;');
    }).mouseout(function(){
        tooltip.hide();
    }).click(function(){
        printCalendar();
    });
});
</script>

</head>
<body>

    <!-- container start --> 
    <div id="container">

        <!-- header start --> 
        <header>
            <figure>
                <img src="headerImg.png" width="600" height="100" alt="header image">
            </figure>
            <div class="clear">&nbsp;</div>
            <nav>
                <!-- header code goes here - this is actually an include file -->
            </nav>
        </header>       
        <!-- header end -->

        <nav>
            <!-- primary navigation start -->
            <div class="clear">&nbsp;</div>
            test
            <!-- primary navigation end -->
        </nav>

        <!-- Application Name start -->
        <div id="applicationName">mySchedule <a href="#" id="whatsthis"><img src="images/what-this.png" width="21" height="21" alt=""></a>
            <!-- breadcrumbs start --> 
            <nav>
                <div id="breadcrumbs">
                    <ul>
                        <li><a href="#MyConnections">Home</a></li>
                        <li>&rarr;</li>
                        <li><a href="#Exhibitors">mySchedule</a></li>
                    </ul>
                </div>
            </nav>
            <!-- breadcrumbs end --> 
        </div>
        <!-- Application Name end -->

        <!-- Features Toolbar start -->
        <div id="features" class="right">
            <div class="featuresAddOn"> <a href="#" id="printthis">Print mySchedule</a></div>
        </div>
        <!-- Features Toolbar end -->

        <div class="clear">&nbsp;</div> 

        <!-- Left Side Bar start -->
        <div class="left">
            <nav>
                <!-- Application Navigation start -->
                <div id="appMenuContainer">
                    <div class="appMenu">
                        <h4 class="firstitem"><a href="/jsp/Myclients/Myclients.jsp?action=ClientMyMatchTab">Client Matching</a></h4>
                    </div>
                    <div class="appMenu">
                        <h4 id="MyClient" class="menuitem header"><span><a href="/jsp/Myclients/Myclients.jsp">MyClients</a></span></h4>
                    </div>
                    <div class="appMenu">
                        <h4 id="MySched" class="menuitem header"> <span><a style ="cursor:default">MySchedule</a></span></h4>
                    </div>
                    <div class="appMenu">
                        <h4 id="MyHel" class="menuitem lastitem"> <span><a href="/jsp/Myclients/help.jsp">Help</a></span></h4>
                    </div>
                </div>
                <!-- Application Navigation end -->
            <nav>

            <!-- Alert Box start -->
            <aside>
                <div id="AlertBox" class="margin-bottom margin-top">
                    <div class="title">Alert</div>
                    <div class="contentAlert lastItem"> You have <span class="count New_Time">0</span> New Time Proposed Notifications. </div>
                </div>
            </aside>
            <!-- Alert Box ends -->
        </div>
        <!-- Left Side Bar end -->

        <section>
            <!-- Content -->
            <div id="calendar"></div>
        </section>

        <div class="clear">&nbsp;</div>

    </div>
    <!-- container end --> 

    <footer>
        © MyCompany, Inc. 2011
    </footer>

</body>
</html>
  • Among the requirements of the project are that the website use HTML5, and support IE7, IE8, and IE9.
  • I have more than one nav. Do each of these require an id?
  • This is one page of several. The content on all of the pages appears within the div id="calendar" tag. This is dynamically replaced depending on the client. This page looks like a calendar/schedule, for which clients can set appointments.
  • I encased the alert box underneath the left side navigation in aside tags. The alert box content is related to the calendar content - it alerts a client if an appointment has been set on the calendar - but wouldn't make sense if it stood on it's own. Is this use of the aside tag correct in this instance?
  • Regarding the header and footer, previously I had divs that had styles attached to them. Since I removed them, I'll now add the styles to the header and footer tags. Is that the correct way to do this? If not, where do the styles go?

BTW, I'm a web development contractor brought in on this project. So I'm basically updating existing code.

Thanks.

Stephen

share|improve this question

1 Answer 1

  • give the site heading in a h1 (probably in header); if the "headerImg.png" is your site name/logo, enclose it in h1 instead (and give the site name in alt)
  • if your "headerImg.png" is a typical site header image, you shouldn't use figure
  • <div class="clear">&nbsp;</div> is no good style. You can probably use CSS instead.
  • it's not clear to me why you have one nav inside of header and one after. Is the one in header not for major navigation of the site? Then you shouldn't use nav for it
  • <li>&rarr;</li>: don't add this on its own li (better use CSS for it)
  • <div class="title">Alert</div>: use a heading instead (you can use h1 here)
  • you could enclose "© MyCompany, Inc. 2011" in a small element (inside of footer)

Is "applicationName" some kind of (main) content on the page? In this case you should give a heading here and/or enclose it in a sectioning element (section resp. article). Otherwise the Left Side Bar nav would be in the scope of the whole page.

Because your aside is related to the main content in section, the aside should be inside of that section. At the moment it is related to the whole page.


I have more than one nav. Do each of these require an id?

You don't have to use an id for nav. Only use it if you need/want page anchors (so users can jump to a certain place).

Regarding the header and footer, previously I had divs that had styles attached to them. Since I removed them, I'll now add the styles to the header and footer tags. Is that the correct way to do this? If not, where do the styles go?

Yes, you can (and probably should) style the elements directly. Only use div for styling if you need to group several elements.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.