1
\$\begingroup\$

I'm doing an exercise where I pick an existing webpage and I code its front end (in HTML and CSS) from scratch, just by looking at it. I started off with an easy one. It would be great if somebody could review my code.

  • What are the best units to use? I know px is discouraged. Should I replace them with em and rem? How do I know where to use em and where to use rem?
  • In CSS, is one line per property good, or is it better to go more compact?
  • I declare each selector only once. I don't use multiple selectors and commas, I feel that leads to that selector's code getting spread out all over the place. Is my way good practice?
  • Are my names for ID's, classes, etc OK?
  • Feel free to make other suggestions.

Note: The original website has working menus and is mobile friendly. My copy is not. I kept it simple.

Original Website

https://www.titanvolunteers.com/

screenshot of volunteer registration website's home page

My Copy

body {
    background-color: #999999;
    margin-bottom: 0;
}

#page-container {
    background-color: #DFDFDF;
    width: 1150px;
    /* is margin: auto the best way to center a display: block? */
    margin: 12px auto 0 auto;
}

#page-container2 {
    padding: 12px 12px 0 12px;
}

header {
    background-color: white;
}

nav {
    background-color: black;
    color: white;
}

article {
    background-color: #F5F5F5;
    color: #666666;
}

#site-name-container {
    /* No floating needed. It sits on the left automatically. */
    text-align: center;
    width: 360px;
    padding: 20px 10px 20px 10px;
}

#site-name {
    color: #059BD8;
    font-size: 2.5rem;
}

#tagline {
    color: #6E6C6A;
    font-family: sans-serif;
}

.category {
    display: inline-block;
    padding: 18px 25px;
    font-family: sans-serif;
}

.category:hover {
    background-color: #059BD8;
}

#hero-image {
    width: 580px;
}

#mid-page {
    background-color: #F5F5F5;
    /* Is display: flex the best way to get 2 fixed width blocks side by side? Or better to use something else? */
    display: flex;
}

article {
    /* padding-bottom: 1px here because if I set it to 0, the p margin-bottom disappears. Is there a better way to do this? */
    padding: 20px 20px 1px 20px;
    width: 670px;
}

aside {
    padding: 20px 0 0 40px;
    background-color: #F5F5F5;
    color: #666666;
    width: 360px;
}

h1 {
    border-bottom: 1px solid #666666;
    font-weight: normal;
}

p {
    font-family: sans-serif;
}

h2 {
    font-weight: bold;
    font-style: italic;
    text-decoration: underline;
    font-size: 1rem;
    font-family: sans-serif;
}

aside > h1 {
    border: 0;
    border-bottom: 1px solid #666666;
    padding-bottom: 15px;
    margin-bottom: 0;
}

.sidebar-event {
    border-bottom: 1px solid #666666;
    padding: 15px 0;
    display: flex;
}

.event-logo {
    display: flex;
    justify-content: center;
    align-items: center;
    background-color: white;
    border: 1px solid #666666;
    width: 130px;
    height: 130px;
}

.event-logo > img {
    width: 90%;
    display: block;
}

a {
    color: #059BD8;
    font-family: sans-serif;
}

a:hover {
    background-color: darkorange;
}

.event-description {
    width: 200px;
    margin-left: 10px;
}

.event-description > p {
    margin: 0;
}

.event-description > a {
    font-weight: bold;
}

footer {
    background-color: #3D3D3D;
    color: white;
    font-family: sans-serif;
    padding: 12px 25px;
    display: flex;
    justify-content: space-between;
}
<!DOCTYPE html>

<html lang="en-us">

<head>
    <title>TitanVolunteers.com - Volunteer Registration For Events</title>
    <link rel="stylesheet" href="style.css">
</head>

<body>
    <div id="page-container">
        <div id="page-container2">
            <header>
                <div id="site-name-container">
                    <div id="site-name">
                        TitanVolunteers.com
                    </div>
                    <div id="tagline">
                        Volunteer Registration For Events
                    </div>
                </div>
            </header>
            
            <nav>
                <div class="category">
                    Home
                </div>
                <div class="category">
                    Sign Up To Volunteer
                </div>
                <div class="category">
                    Volunteer Coordinators
                    <!-- TODO: get hover to display a sub menu. I checked, it's all CSS, no JS -->
                </div>
                <div class="category">
                    Control Panel
                </div>
                <div class="category">
                    About Us
                </div>
                <div class="category">
                    Contact Us
                </div>
            </nav>
            
            <div id="mid-page">
                <article>
                    <img id="hero-image" src="https://www.titanvolunteers.com/assets/images/liquid_run_volunteers.jpg">
                    <h1>
                        Welcome!
                    </h1>
                    <p>
                        Welcome to Titan Volunteers, your one stop shop for everything related to volunteering for events. Whether you're a volunteer looking for a great event, or an event manager who needs the right tools to collect and track signups for an event, we've got you covered.
                    </p>
                    <h2>
                        Getting Started
                    </h2>
                    <p>
                        Volunteers are encouraged to look for events via our <a href="https://www.titanvolunteers.com/volunteers/view_event_list">sign up to volunteer</a> page. Volunteer coordinators are encouraged to check out our <a href="https://www.titanvolunteers.com/general/screenshots">features page</a> and <a href="https://www.titanvolunteers.com/general/sign_up_company"</a>create a free account</a>.
                    </p>
                    <h2>
                        Need Help?
                    </h2>
                    <p>
                        If you have any questions or issues with anything on the website, we're here to help! Simply use our Contact Us page and we will assist you right away.
                    </p>
                    <h2>
                        Spread The Word!
                    </h2>
                    <p>
                        Please tell your friends about us. We are always looking to help out more people. Thanks a lot and enjoy the site.
                    </p>
                </article>
                <aside>
                    <h1>
                        Volunteer at these great events
                    </h1>
                    
                    <div class="sidebar-event">
                        <div class="event-logo">
                            <img src="https://www.titanvolunteers.com/uploads/race_logos/E351-Herbalife24-Triathlon-Los-Angeles-2020-1574926089.png">
                        </div>
                        <div class="event-description">
                            <a href="https://www.titanvolunteers.com/volunteers/sign_up_individual/416-Herbalife24-Triathlon-Los-Angeles-2020">
                                Herbalife24 Triathlon Los Angeles 2020
                            </a>
                            <p>
                                Our second annual event provides a one-of-a-kind journey through the heart of LA. Participants start in Venice Beach and end in Downtown Los Angeles at L.A ...
                            </p>
                        </div>
                    </div>
                    <div class="sidebar-event">
                        <div class="event-logo">
                            <img src="https://gallery.mailchimp.com/b03d2aea85ec993d0dd85362d/images/d2a54ce5-4e35-4c9c-844e-4cc4685e08ef.png">
                        </div>
                        <div class="event-description">
                            <a href="https://www.titanvolunteers.com/volunteers/sign_up_individual/355-SDCCU-OC-Marathon-OC-Half-Marathon">
                                SDCCU OC Marathon & OC Half Marathon
                            </a>
                            <p>
                                The SDCCU OC Marathon would not be possible without the support of over 2,000 volunteers that assist throughout the event weekend. We will host and suppor ...
                            </p>
                        </div>
                    </div>
                </aside>
            </div>
        </div>
        <footer>
            <div id="copyright">
                Copyright © 2016-2020 TitanVolunteers.com. All Rights Reserved.
            </div>
            <div id="template">
                Template by <a href="http://www.os-templates.com/">OS Templates</a>
            </div>
        </footer>
    </div>
</body>

</html>

\$\endgroup\$
1
\$\begingroup\$

I will focus on the CSS file.

Keep the CSS file organized, whether it be class first, then ID, then element tags (p, a, nav etc), alphabetical or some other logical organization.

One of the lines you have is background-color: darkorange;. Without the quotes, it looks odd to me (although allowed by the specification). Also, why is this the one of the few that are not in hex notation?

Keep the definitions consistent (padding always before display or display always before padding or some other logical grouping and organization):

.category {
    display: inline-block;
    padding: 18px 25px;
    font-family: sans-serif;
}

.sidebar-event {
    border-bottom: 1px solid #666666;
    padding: 15px 0;
    display: flex;
}

In the HTML file, there is one/two extraneous </a> (at the very end):

Volunteers are encouraged to look for events via our <a href="https://www.titanvolunteers.com/volunteers/view_event_list">sign up to volunteer</a> page. Volunteer coordinators are encouraged to check out our <a href="https://www.titanvolunteers.com/general/screenshots">features page</a> and <a href="https://www.titanvolunteers.com/general/sign_up_company"</a>create a free account</a>.

This might be a copy/paste error for <a href="https://www.titanvolunteers.com/general/sign_up_company"</a>.

I'm not sure about the legalities of copying the website design and posting it here (see https://codereview.stackexchange.com/help/licensing).

\$\endgroup\$
1
  • \$\begingroup\$ Darkorange wasn't in hex notation because I couldn't use my color grabber tool on it because the color only appears on hover. I was lazy, didn't want to go into Chrome Inspect Element and manually trigger the hover. That stray </a> tag might be my code editor adding it incorrectly when I was typing/pasting stuff. I own that website, I promise not to sue Code Review. Thanks for the tips! For keeping properties in a consistent order, maybe I should just alphabetize them? \$\endgroup\$ – RedDragonWebDesign Jul 22 '20 at 3:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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