1
\$\begingroup\$

This is a single page HTML/CSS layout. This is my first ever code and I know it's not perfect, but I would love for someone with a lot more experience to let me know of my mishaps.

<!DOCTYPE HTML>
<html>

<head>
    <link type="text/css" rel="stylesheet" href="dereke.css"/>
    <title>

    </title>
</head>

<body>
    <div class="Header">
        <p>Ohio State Buckeyes</p>
        <div id="logo">
            <img src="http://vignette2.wikia.nocookie.net/logopedia/images/6/6f/1000px-Ohio_State_Buckeyes_logo_svg.png/revision/latest?cb=20130425230958" />
        </div>
        <div id="navbar">
            <ul>
                <li>Home</li>
                <li id="noUD">|</li>
                <li>About Us</li>
                <li id="noUD">|</li>
                <li>Contact Us</li>
            </ul>
        </div>
    </div>
    <div id="leftbody">
            <img src="http://printableteamschedules.com/images/collegefootball/ohiostatebuckeyesfootballschedule.gif" />
        </div>
        <div id="rightbody"></div>
    <div id="lowerbody">
            <img src="http://grfx.cstv.com/schools/osu/graphics/facilities/stadium-night-800x325.jpg" />
        </div>

    <div class="footer">
         <p id="footword">Ohio State is the best college in the United States! 2016</p>
    </div>

</body>

</html>

CSS

.Header {
    width: calc(100%-16px);
    height: 150px;
    border-radius: 5px;
}
.Header p {
    color: white;
    margin-top: -5px;
    width: 600px;
    font-size: 70px;
}
.MidBody {
    background-color: #141414;
    width: 100%;
    height: 850px;
    margin-top: 10px;
    border-radius: 5px;
    position: relative;
    display:block;

}
.footer {
    bottom: 0;
    height:50px;
    width: 100%;
    margin-top: 10px;
    border-radius: 5px;
    clear:both;
    position:relative;
    text-align:center;
    color: white;
}
#leftbody {
    width: 49%;
    height: 425px;
    left: 0;
    margin-top: 3px;
    margin-left: 3px;
    position: relative;
    z-index: 1;
    border-radius: 5px;
    float:left;
}
#leftbody img {
    width: 490px;
    height: 420px;
    border-radius: 5px;
    margin-top: 2px;
    margin-left: 2px;
}
#rightbody {
    background-color: #F1F1F1;
    width: 49%;
    height: 425px;
    margin-top: 3px;
    margin-right: 3px;
    position: relative;
    z-index: 1;
    border-radius: 5px;
    float:right;
}
#lowerbody {
    width: 100%;
    height:auto;
    left:0;
    right:0;
    margin-left: auto;
    margin-right: auto;
    postion: relative;
    display: inline-block;
    text-align:center;
}
#lowerbody img {
    position:relative;
    border-radius: 5px;
    margin-left: auto;
    margin-right: auto;
}
body {
    background-color: black;
}
li {
    display: inline;
    padding: 1px;
    text-decoration: underline;
}
#navbar {
    width: 350px;
    color: #F8F8F2;
    font-family: Arial Black;
    margin: -35px;
    text-align: left;
    line-height: 10px;
}
#noUD {
    text-decoration: none;
}
#logo img {
    margin-top: -150px;
    margin-right: 50px;
    width: 15%;
    height: 15%;
    float: right;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! So far, you have not explained what this code is doing. Please provide a minimal description in the title (not a question), and provide further details in the body. \$\endgroup\$ – SirPython Mar 13 '16 at 2:53
  • \$\begingroup\$ Ok Im new here. Sorry about that. \$\endgroup\$ – Derek Edwards Mar 13 '16 at 2:55
1
\$\begingroup\$

You have some repeat id tags which aren't good. Using class names instead is much better.

<ul>
   <li>Home</li>
   <li class="noUD">|</li>
   <li>About Us</li>
   <li class="noUD">|</li>
   <li>Contact Us</li>
</ul>

And in the CSS it becomes:

.noUD {
  text-decoration: none;
}

You also have standard HTML elements for headers, footers, navigations, and articles:

  • <div class="Header"> becomes <header>
  • <div class="Footer"> becomes <footer>
  • <div id="navbar"> becomes <nav>

You are still able to keep classes and ids with this. It just adds extra readability for systems (and humans) and it makes it better in case your CSS fails because browsers apply their own styling to some standard elements.


position: relative; bottom: 0 does nothing in terms of positioning because it sets the element 0 up from where it was before.

Other than that the code looks pretty good.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much. Still learning. I really appreciate you taking the time to check it out! \$\endgroup\$ – Derek Edwards Mar 27 '16 at 4:23
  • \$\begingroup\$ @DerekEdwards no problem. Glad I could help. \$\endgroup\$ – CKH4 Mar 27 '16 at 4:27
0
\$\begingroup\$

You write down your CSS properties in a random order. This is okay for small projects but can be tedious to maintain very quickly.

Create an order, in wich you set the properties for an element, and stick to it. It should look something like:

  1. Color
  2. Size (width, height)
  3. Position (position, bottom, top...)
  4. Margins, paddings
  5. Styling (borders, shadows...)
  6. Etc. (animations, box-sizing...)

Then the code for this specific one would look like

#element {
    background-color: #007788;
    width: 800px;
    height:200px;
    position: absolute;
    left: 100px;
    top: 10px;
    margin: 10px;
    padding: 5px;
    border-bottom: 4px solid #003355;
    border-radius: 5px;
    box-shadow: 0 0 10px grey;
    box-sizing: border-box;
}

You don't need to include a property from every option.


You should also break your CSS into multiple classes, for example you have the border-radius: 6px in a lot of places, that could have a separate class for it.


\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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