6
\$\begingroup\$

I had a task to create a responsive HTML5/CSS3 page based on PSD layout. I got rejected and when asked for details I got these comments:

  • semantically incorrect HTML
  • incorrect use of ID selectors in CSS
  • incorrect general way of solving given CSS problems

The link is here

HTML:

<!doctype html>
<html lang="hr">
<head>
    <meta charset="utf-8" />
    <title>Lorem ipsum</title>
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1" />
    <link rel="stylesheet" href="css/reset.css" />
    <link rel="stylesheet" href="css/style.css" />
    <link href='http://fonts.googleapis.com/css?family=Open+Sans&amp;subset=latin,latin-ext' rel='stylesheet' type='text/css' />
    <link rel="stylesheet" href="js/jquery.bxslider/jquery.bxslider.css" />

    <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js"></script>
    <script src="js/jquery.bxslider/jquery.bxslider.min.js" type="text/javascript"></script>
    <script src="http://maps.googleapis.com/maps/api/js?&sensor=false" type="text/javascript"></script> 
    <script src="js/script.js" type="text/javascript"></script>

    <!--[if lt IE 8]>
       <div style=' clear: both; text-align:center; position: relative;'>
         <a href="http://windows.microsoft.com/en-US/internet-explorer/products/ie/home?ocid=ie6_countdown_bannercode">
           <img src="http://storage.ie6countdown.com/assets/100/images/banners/warning_bar_0000_us.jpg" border="0" height="42" width="820" alt="You are using an outdated browser. For a faster, safer browsing experience, upgrade for free today." />
        </a>
      </div>
    <![endif]-->
    <!--[if lt IE 9]>
        <script src="js/html5.js"></script>
        <link rel="stylesheet" href="css/ie.css" /> 
    <![endif]-->

</head>
<body>





<div id="divMain" class="div_base">
    <!-- header -->
    <div id="divHeaderHolder" class="div_base">
        <header class="div_base">

            <div id="divLogo">
                <a href="#"><img src="images/logo.png" alt="" title="" /></a>
            </div>
            <div id="divHeadRight">

                <div class="head_tel">Toll Free Number: <a href="tel:0800000000" class="a_head_tel">0800 00 00 00</a></div>
                <div class="head_links_holder">                 
                    <a href="#" id="aLocate">Locate me</a>
                    <a href="#" id="aProfil">My Profile</a>
                    <a href="#" id="aFB"></a>
                    <a href="#" id="aTW"></a>
                </div>

            </div>

        </header>


        <nav class="div_base">

            <a href="#" id="aHome" class="underline">Home</a>
            <a href="#" class="underline menu_color menu_bg_color">Page Link 1</a>
            <a href="#" class="underline menu_color menu_bg_color">Page Link 2</a>
            <a href="#" class="underline menu_color menu_bg_color">Page Link 3</a>
            <div id="divMenuMobile"><a href="#" id="aMenuMobile"></a></div>
            <form>
            <input type="search" id="txt_search" placeholder="Search..." class="div_base menu_color" />
            </form>
            <div id="divAutocomplete"></div>

        </nav>



    </div>
    <!-- /header -->



    <!-- main slider -->
    <div id="divSliderMainHolder" class="div_base">
        <div id="divSliderMain" class="div_base">

            <ul id="ulSliderMain">
              <li><img src="images/slider1.jpg" alt="" /></li>
              <li><img src="images/slider1.jpg" alt="" /></li>
              <li><img src="images/slider1.jpg" alt="" /></li>
              <li><img src="images/slider1.jpg" alt="" /></li>
            </ul>   


        </div>
    </div>
    <!-- /main slider -->


    <!-- content -->
    <div id="divContentHolder" class="div_base">    
        <section class="div_base">


            <div id="divCnt" class="div_base">


                <div class="news_item">

                    <div class="news_imgholder">
                        <a href="#"><img src="images/news_img1.jpg" alt="" /></a>
                    </div>

                    <div class="news_date">17.04.2014.</div>
                    <div class="news_title"><a href="#" class="underline">Typi non habent claritatem insitam</a></div>
                    <div class="news_descr">
                        čćžšđ ČĆŽŠĐ Lorem ipsum dolor sit amet, consectetuer adipiscing elit, sed...                
                    </div>

                    <div class="news_bottom">                   
                        <div class="news_bottom_left div_base"></div>
                        <div class="news_bottom_right">
                        <a href="#" class="news_more"></a>
                        </div>
                    </div>

                </div>


                <div class="news_item">

                    <div class="news_imgholder">
                        <a href="#"><img src="images/news_img2.jpg" alt="" /></a>
                    </div>                      

                    <div class="news_date">02.12.2013.</div>
                    <div class="news_title"><a href="#" class="underline">Typi non habent claritatem insitam; est usus legentis in iis qui facit eorum claritatem.</a></div>
                    <div class="news_descr">
                        Lorem ipsum dolor sit amet, <a href="#">consectetuer adipiscing elit</a>, sed 
                        enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis dolore te feugait nulla facilisi.
                    </div>

                    <div class="news_bottom">                   
                        <div class="news_bottom_left div_base"></div>
                        <div class="news_bottom_right">
                        <a href="#" class="news_more"></a>
                        </div>
                    </div>              

                </div>              




                <div class="news_item">

                    <div class="news_imgholder">
                        <a href="#"><img src="images/news_img3.jpg" alt="" /></a>
                    </div>

                    <div class="news_date">01.12.2013.</div>
                    <div class="news_title"><a href="#" class="underline">Typi non habent claritatem insitam</a></div>
                    <div class="news_descr">
                        Lorem ipsum dolor sit amet, consectetuer adipiscing elit, sed...                
                    </div>

                    <div class="news_bottom">                   
                        <div class="news_bottom_left div_base"></div>
                        <div class="news_bottom_right">
                        <a href="#" class="news_more"></a>
                        </div>
                    </div>                  

                </div>      



            </div>



            <div id="divSidebar">

                <div id="divSideSlider">
                    <div class="sidebar_item">
                        <img src="images/sidebar1.jpg" alt="" />
                    </div>
                    <div class="sidebar_item">
                        <img src="images/sidebar2.jpg" alt="" />
                    </div>              
                </div>

            </div>


        </section>

    </div>
    <!-- /content -->


    <!-- footer -->
    <div id="divFooterHolder" class="div_base">
        <footer class="div_base">

            <div id="divFooterLeft">
                This site is powered by...
            </div>

            <div id="divFooterRight">
                Design & Technology: D.S.
            </div>

        </footer>
    </div>


</div>

<div id="RWDMenu"></div>


<!-- some general stuff: back to top, custom alert box, ajax preloader... -->

<a href="#top" id="aBackTop"></a>

<div id="divAlertHolder" class="div_base"><div id="divAlert"></div></div>

<div id="divPreloaderholder" class="div_base"><div id="divPreloader"></div></div>


</body>
</html>

CSS:

*  {
    -moz-box-sizing: border-box;
    box-sizing: border-box; 
}

body {  
    font-family: 'Open Sans', sans-serif;
    font-size: 16px;
    background: #fff;
}

img, embed, object, video {
    max-width: 100%;
}


a {
    text-decoration: none; 
    color: #000;
}

a:hover {
    text-decoration: underline;
}

.underline:hover  {
    border-bottom: 2px solid #C4C8CC;
    text-decoration: none;
}

/* for various elements due to OOCSS */

.div_base {
    position: relative;
    width: 100%;
}

.menu_color {
    color: #8F8F8F;
}

.menu_bg_color {
    background-color: #F4F4F4;
}




/* ===================================== */

/* all main content, menus, footer... are grouped in divMain in order to slide left navigation */

#divMain {
    margin: 0 auto; 
    z-index: 20;
} 

/* this is menu below for smartphones */

#RWDMenu {
    position: absolute;
    top: 0; left: 0;
    z-index: 10;    
}


/* ===================================== */

/* Header */

#divHeaderHolder {
    height: auto;
    background-color: #333333;
    -webkit-box-shadow: 0px 18px 20px 0px rgba(0, 0, 0, 0.25);
    box-shadow: 0px 18px 20px 0px rgba(0, 0, 0, 0.25);
    z-index: 30;
}

header {
    margin: 0 auto; 
    max-width: 1144px;
    min-height: 107px;
    height: auto;
    color: #fff; 
    font-size: 11px;
}

#divLogo {  
    position: relative; 
    display: block; 
    float: left;
    padding-top: 17px;
}

#divHeadRight { 
    position: relative; 
    float: right;
    padding: 17px 19px 0 0;
    font-size: 11px;
    color: #fff;
    text-transform: uppercase;  
}

#divHeadRight a {
    color: #fff;
}

.head_tel {
    position: relative;
    color: #F0F0F0;
    text-align: right;
}

.a_head_tel {
    position: relative;
    top: -5px;
    font-size: 16px;    
}
.a_head_tel:hover {
    text-decoration: none;
}

.head_links_holder {
    position: relative;
    display: block; 
    margin-top: 25px;   
    text-align: right;  
}



#aLocate {
    display: inline-block;
    position: relative;
    margin-right: 17px;
    background: url(../images/icon_googlemap.png) no-repeat right top; 
    height: 30px;
    padding: 7px 30px 0 0;
}

#aLocate:hover {
    background-position: right -30px;
} 

#aProfil {
    display: inline-block;
    margin-right: 42px;
    padding-top: 7px;   
}

#aFB  {
    display: inline-block;
    position: relative;
    background: url(../images/icon_fb.png) no-repeat 0 0; 
    width: 30px; height: 30px;
    margin-right: 14px;
    text-indent: -9999px; 
}
#aFB:hover {
    background-position: 0 -30px;
}   

#aTW  {
    display: inline-block;
    position: relative;
    background: url(../images/icon_tw.png) no-repeat 0 0; 
    width: 30px; height: 30px;
    text-indent: -9999px; 
}
#aTW:hover {
    background-position: 0 -30px;
}   


nav {
    display: table; 
    max-width: 1144px;
    height: 52px;
    margin: 0 auto;     
}

nav > a {
    display: table-cell;
    width: 150px;
    white-space: nowrap;
    height: 52px;   
    padding: 14px 50px 12px 50px; 
    border-right: 1px solid #C2C2C2;
    font-size: 18px; 
    text-transform: uppercase;
}

#aMenuMobile {
    display: block;
    background: url(../images/menu_mobile.png) no-repeat 0 0; 
    width: 25px; height: 25px;
    text-indent: -9999px;
}

#divMenuMobile {
    display: none;
}

#aHome  {
    display: table-cell;
    background: url(../images/icon_home.png) no-repeat 0 0; 
    width: 106px; height: 52px;
    text-indent: -9999px; 
}
#aHome:hover:after {
    content:'';
    position: absolute;
    top: 0; left: 0; 
    width: 106px; height: 52px;
    background: rgba(0, 0, 0, 0.2);
}   


#txt_search {
    height: 52px;
    background: #EEE9E9;
    box-shadow: inset 7px 7px 15px 0px rgba(0, 0, 0, 0.25);
    border: 0;
    font-style: italic; 
    font-size: 24px; 
    padding: 15px;
}

#divAutocomplete {
    position:absolute; 
    display: none; 
    width: 500px; height: 200px; 
    border: 1px solid #ccc; 
    background-color: #fff; 
}

.ul_autocomplete {
    list-style: none;
    padding: 0;
    margin: 0;
}

.li_autocomplete {
    padding: 5px; 
    background-color: #ABABAB; 
    color:#000; 
    border-bottom: 1px solid #ccc; 
    cursor: pointer;
}
.li_autocomplete:hover {
    background-color: #000; 
    color:#fff; 
}





/* Main Slider */

#divSliderMainHolder {
    background-color: #fff;
}

#divSliderMain {
    max-width: 1144px;
    padding: 31px 28px 44px 28px;
    margin: 0 auto;
}



/* Content elements */ 

#divContentHolder {
    margin-top: -50px;
    background-color: #F9F8F6;
    border-top: 1px solid #D9D9D8;
    border-bottom: 1px solid #D9D9D8;
}

section {
    display: table;
    max-width: 1144px;  
    padding: 40px 28px 47px 28px;
    margin: 0 auto; 
}



#divCnt {
    display: table-cell;
    max-width: 805px;
    padding-right: 28px;
}

    /* news */

.news_item {
    clear: both;
    min-height: 176px;
    margin-bottom: 29px;
}

.news_imgholder {
    float: left;
    width: 158px; height: 142px;
    margin: 0 27px 10px 0;
    padding-left: 16px;
    background-color: #EEEEEE;
    overflow: hidden;
}

.news_imgholder  img:hover {
    opacity: .7;
}

.news_date {
    color: #ABABAB;
    font-size: 13px;
    font-weight: bold;
}

.news_title {
    color: #000;
    font-size: 24px;
    line-height: 35px;  
    margin-bottom: 10px;    
}

.news_descr {
    color: #6C6C6C;
    font-size: 16px;    
    line-height: 25px;  
}

.news_more  {
    display: block;
    position: relative; 
    background: url(../images/icon_more.png) no-repeat 0 0; 
    width: 42px; height: 42px;
    text-indent: -9999px; 
}
.news_more:hover:after {
    content:'';
    position: absolute;
    top: 0; left: 0; 
    width: 42px; height: 42px;
    background: rgba(0, 0, 0, 0.2);
}   


.news_bottom {
    display: table;     
}

.news_bottom_left {
    display: table-cell; 
    height: 42px; 
    border-bottom: solid 1px #C4C8CC; 
}

.news_bottom_right {
    display: table-cell; 
    width: 64px; height: 42px; 
    padding-left: 22px;
}

    /* sidebar */

#divSidebar {
    display: table-cell;
    width: 251px;
}

.sidebar_item {
    margin-bottom: 20px;
    width: 251px;
}



/* Footer */

#divFooterHolder {
    background-color: #fff;
}

footer {
    margin: 0 auto;    
    display: table;
    max-width: 1144px;      
    height: 66px;
}

#divFooterLeft {    
    display: table-cell;
    vertical-align: middle;
    padding-left: 28px;
}

#divFooterRight {   
    display: table-cell;
    vertical-align: middle;
    text-align: right;
    padding-right: 28px;
}




/* some general stuff: back to top, custom alert box, ajax preloader... */

#aBackTop  {
    display: none;
    position: fixed;
    right: 10px; bottom: 50px;
    background: url(../images/icon_up.png) no-repeat 0 0; 
    width: 42px; height: 42px;
    text-indent: -9999px; 
    margin: 0;
    z-index: 99;
}
#aBackTop:hover:after {
    content:'';
    position: absolute;
    top: 0; left: 0; 
    width: 42px; height: 42px;
    background: rgba(0, 0, 0, 0.2);
}   


#divAlertHolder {
    position: fixed; 
    top: 0; left: 0; 
    z-index: 99;
}
#divAlert {
    display: none;
    position: relative;
    margin: 100px auto;
    width: 80%; height: 400px;  
    color: #000;
    background-color: #fff;
    border: 1px solid #C4C8CC;
    -webkit-box-shadow: 4px 4px 15px #333;
    box-shadow: 4px 4px 15px #333;
    text-align: center;
}

#divPreloaderholder {
    position: fixed; 
    top: 0; left: 0; 
    z-index: 99;
} 
#divPreloader {
    display: none;
    position: relative;
    margin: 100px auto;
    width: 48px; height: 48px;  
    background: url(../images/preloader.gif) no-repeat 0 0; 
}



@media screen and (max-width: 1024px) {
/* ipad horizontal */
/* menu bar appears for mobile */

    nav > a, #aHome {
        display: none;
    }
    #divMenuMobile {
        display: table-cell;
        width: 52px; height: 52px;
        padding: 13px;
    }

    #txt_search {
        display: block;
        width: 40%;
        min-width: 207px;
    }   

    #RWDMenu a {
        display: block;
        width: 250px;
        white-space: normal;
        min-height: 52px; height: auto;
        padding: 14px 16px 12px 16px; 
        border-right: 1px solid #C2C2C2;
        border-bottom: 1px solid #C2C2C2;
        font-size: 18px; 
        text-transform: uppercase;  
    }

}



@media screen and (max-width: 700px) {
/* head right elements go below */


    #divLogo {  
        float: none;
        clear: both;
    }

    #divHeadRight { 
        float: none;
        clear: both;
        margin-left: 16px;
    }

    .head_tel {
        text-align: left;
    }

    .a_head_tel {
        font-size: 16px;
    }


    .head_links_holder {
        margin: 10px 0 10px 0;
        text-align: left;
    }


    #aLocate, #aProfil, #aFB, #aTW {
        margin-right: 3px;
    }   

    #divCnt {
        display: block;
        padding-right: 0;
    }

    #divSidebar {
        display: block;
    }

    footer {
        font-size: 12px;
    }

    #divAlert {
        margin-top: 200px;
        height: 250px;
    }

}

Maybe I could use article instead of section or within a section, but is it really such a mistake, or I don't see a big picture? Please help me out by pointing out what I do wrong, because obviously I am going in a wrong direction in learning HTML5/CSS3, and if it's not too wide issue, what should I correct?

Improve this question
\$\endgroup\$
0

1 Answer 1

Reset to default
12
\$\begingroup\$

Several things that would cause me to reject this:

  • Nearly every element on the page is a <div>.

    <div> is semantically void. It says nothing about the structure of the document, other than that "there's a block here". (Except that they're not quite even that, cause you've repurposed a few of them as table cells.)

    A huge number of those divs could be replaced with elements that are more semantically correct.

    • Those <div class="news_item"> could be <article>s.
    • For lists of links (like in your header)...note how i said "list". :P <nav> would work too.
    • <div class="news_date"> could be a <time>.

    You also have "container" elements that by design only contain one element -- which itself is a container. I tend to consider that broken, except in very rare cases.

  • Your use of IDs and class names is broken.

    • I can almost forgive giving nearly every freaking element in the page an ID and/or class. Not quite, but almost. In my opinion, there should be a reason to distinguish these elements. Don't give stuff an ID or class just because you can. It adds noise.

      And the performance argument is an example of premature optimization. You're mucking up the HTML over a couple of milliseconds at best. Was your page slow without all those IDs? (Hint: No.) Don't "optimize" for the sake of doing so, or because some schmuck online said this is how $BIG_COMPANY does it, or whatever. Do it because you've determined it matters in your case. When it matters.

    The whole point of separating content from presentation, though, is so that the two don't get intertwined. HTML structures the document, CSS determines how it looks, and either one can change independently of the other.

    And with this code:

    • The class names are often presentational. underline? Really? So if i don't want links to be underlined, i have to edit that class name out of the HTML?

      The problem here is that you're embedding assumptions about formatting. There is a reason you want these links underlined. Boil that reason down to a class name and use that instead.

    • What's worse, your IDs embed the actual element names in them. So if i ever do go to make this less of a semantic mess, now i have to go and edit the CSS too.

    You've largely tossed that separation out the window. Now, any significant change -- to either the structure or the layout -- will probably have to be made in two places.

    (By the way, Cnt isn't much of an ID either. Don't abbreviate short single words.)

  • Those conditional chunks of HTML. In your header. I'm not going to say too much about that, cause you're at least trying to help the poor souls still using IE 6-8. But the header is for metadata, scripts, and stylesheets. There shouldn't be any visible content.

  • A minor quibble, because this is obviously a sample. But the logo -- which is solely text -- has no alt text. So a spider or blind person wouldn't even know the company name.

  • Those link-buttons. Let me get this out of the way: it seems broken to me to have a link with no content. Consider what happens if the stylesheet doesn't load, or the user is blind or is actually a search engine's spider. You know what they see? Not a whole lot.

As for the CSS...most of my issues with it are more with the HTML it's trying to style. But:

  • The page links have a hover effect that's not consistent with the rest of the page. Other links like that, you shade them. I was about to call that a bug, til i saw it was intentional. :P Keep it consistent.

  • You do a lot of fiddling around with margins and padding. That'd be OK if the numbers were consistent, or if the reason behind it were obvious, but it looks quite arbitrary -- to the point where in some places, in #divSliderMain for example, it seems like you just tweaked numbers til they fit. That hints to me that this design is quite temperamental, and is going to be a pain if i try to modify it in any significant way.

  • Take a look at the CSS for your header links. All of them share some common properties, and the two social-media icons are nearly identical (the only difference being their background images). Is there really not enough similarity there to come up with some common class?

  • height: auto is the default. If you don't override it elsewhere, then setting it doesn't make much sense. Same with display: block for block elements.

  • position: relative doesn't make much sense in most of the places where you've used it. You should only need it when (1) you want absolute-positioned children to position themselves relative to that element, and/or (2) you want to position the element relative to its normal place in the flow.

  • As for the header links: You have three 30x60 images, one per link. The background repositions on hover, which implies you know about CSS sprites. So why make the user download three separate images?

Improve this answer
\$\endgroup\$
18
  • \$\begingroup\$ CHao, thank you very much for your thorough answer! Since my post is deleted, I'll break it down in 3 short comments: "You also have "container" elements that by design only contain one element -- which itself is a container." It's because I have for example a header that is centered and has fixed width, and I have a header background (holder) that stretches horizontally to infinity. What am I doing wrong here? \$\endgroup\$
    – Dalibor
    Apr 29, 2014 at 19:59
  • \$\begingroup\$ "I can almost forgive..." I put ID on just those elements I'm sure there's just 1 of them. Should I use class instead of ID, even if I'm sure it's just one element on the page? I didn't know it's wrong to apply classes to most elements. If I have 2 or more section, 2 or more article, etc... they should have a class, don't they? Even if I have just one article it could share same font with some other element, wouldn't it be redundant to write that property in CSS for article and that other element, instead of applying class with mutual font to both article and other element. \$\endgroup\$
    – Dalibor
    Apr 29, 2014 at 20:00
  • \$\begingroup\$ "The class names are often presentational. underline? Really?" So is it just wrong naming convention or the logic? My logic is: I wanted to have common definition for links (not to repeat some mutual properties for different links in CSS), and some of the links to be underlined so I added them that class also. Is the logic OK if I change the class name? (I mean, that kind of logic I've learnt in OOCSS - is it wrong?) \$\endgroup\$
    – Dalibor
    Apr 29, 2014 at 20:01
  • \$\begingroup\$ @Dalibor: Updated to address the last comment, and part of the second. As for the first...i haven't looked all that closely at it yet. It just happens to be a code smell to me -- it's a lot like having a list that'll never contain more than one item. \$\endgroup\$
    – cHao
    Apr 29, 2014 at 20:40
  • 1
    \$\begingroup\$ As for "OOCSS": It started as a good idea, from what i can tell. I rather like the foundational "media box" example. (Note how the class names mean something, and the HTML isn't trying to dictate layout. That's how it ought to be.) But at some point it lost its way, and now the way it is practiced/hyped is fundamentally flawed. When someone's adding a class to specifically say "this div is 6 grid-columns wide" or "this text is aligned right", they either forgot or never really understood the concept of separating content from presentation. \$\endgroup\$
    – cHao
    Apr 29, 2014 at 21:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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