3
\$\begingroup\$

I'm new to CSS and HTML and while I've achieved a centering of the logo and navigation links, it feels wrong. I'm developing a site for a small non-profit for a side project. I have a logo next to a few navigation links. I put the logo and navigation links in a list.

Could someone take a look at this and tell me if my CSS is correct? I just think that there's something off. Or that I'm duplicating CSS code.

Here's my JSFiddle to show what I've done.

HTML:

<body>
<nav id="top_nav">
    <ul id="nav_links">
        <li>
            <a href="#">
                <img id="nav_logo" src="logo.png"/>
            </a>
        </li>
        <li><a id="nav_whoweare" class="nav_links_border" href="#">WHO WE ARE</a></li>
        <li ><a id="nav_whatwedo" class="nav_links_border" href="#">WHAT WE DO</a></li>
        <li ><a id="nav_explore" class="nav_links_border" href="#">EXPLORE</a></li>
        <li><a id="nav_donate" href="#">DONATE</a></li>
    </ul>
</nav>

<nav id="bottom_nav">

</nav>

CSS:

#top_nav {
    text-align: center;
    width: 100%;
}

#nav_logo {
    /*display: inline-block;*/
    width: 142px;
    height: 159px;
    padding-right: 20px;
    text-decoration: none;
}

#nav_links li a {
    padding: 15px 20px;
    text-decoration: none;
    font-family: Arial;
    font-size: 24px;
    color: #000;
}

/* When window is resized, doesn't wrap menu*/
#nav_links { 
    display: inline-block;
    padding: 0 0 30px 0; 
    list-style:none;
    text-align:left;
    white-space: nowrap;
}

#nav_links li {
    vertical-align: text-top;
    display: inline-block;
    float: none;
    margin: 0 -3px 0 0;

}
/* ------------------------------------*/

a.nav_links_border {
    border-right: 1px solid #f37f43;
}

#nav_links li a:hover {
    text-decoration: underline;
}

#nav_links > img:hover {
    text-decoration: none;
}

#bottom_nav {
    clear: both;
}
\$\endgroup\$
4
\$\begingroup\$

There is no real "correct" way to go about it; it all depends on how you want it to look. I can't quite speak to that, only the code you posted, and how it fares.

Anyway: Review.

General cleanliness

You should clean up your code (preferably before posting it, since many the following things should be fairly obvious). You've got a bunch of CSS that doesn't do anything:

  • There's a line that's just commented out: Remove it.
  • The #nav_links > img:hover doesn't affect anything. There's no img element immediately descendent from #nav_links
  • The #bottom_nav style (and indeed the entire element!) is pointless. It seems only to exist to clear any floats. But there are no floats. Similarly, the #nav_links li style has a float: none declaration which does nothing.
  • You can skip every single link ID (e.g. nav_whoweare); you're not using them. If you find yourself needing them, then add them. But not before.
  • You don't need width: 100% on the nav element. It's a block element, so it'll do that automatically.
  • You don't need display: inline-block on the ul element.

I'd also advice you to use a more logical order for you CSS rules. Right now it's haphazard:

#nav_links li a
#nav_links            << high-level
#nav_links li
#nav_links li a:hover << low-level

but it would make more sense to mirror the structure of the markup (and just the complexity of the CSS selectors):

#nav_links            << high-level
#nav_links li
#nav_links li a
#nav_links li a:hover << low-level

Class names and IDs

"It should be as simple as possible, but no simpler". In other words, only add stuff when there would otherwise be ambiguity.

For instance, there no reason to give the #nav_links list an ID. It's the only <ul> element inside #top_nav, so you can unambiguously reference it as simply #top_nav ul. Or, if you get rid of the bottom_nav element since it's not doing anything, you're left with simply nav ul. (You might want a bottom nav later, but cross that bridge when you come to it; not now).

Furthermore, don't use classes like nav_links_border. It's given that it's a navigation link, because it's inside the <nav> element, and it's given that it's a link because it's an <a> element. So you CSS rule should target nav a rather than an overly specific class like a.nav_links_border.

What you're left with is just border, but that's not terribly descriptive. Looking at it overall, though, it's also obvious that all the links - with 1 exception - have a border. So, really, you'll want a way to treat the exception differently, rather than add a class to everything else.

I'd suggest giving the <li> containing the logo an ID - simply call it logo. This lets you do two things:

  1. You can style all the list items the same basic way, and only do something different for #logo
  2. You can get rid of the ID on the <img> element, since you can instead reference it as #logo img

I get this:

<nav>
    <ul>
        <li id="logo">
            <a href="#"><img src="logo.png"/></a>
        </li>

        <li><a href="#">WHO WE ARE</a></li>
        <li><a href="#">WHAT WE DO</a></li>
        <li><a href="#">EXPLORE</a></li>
        <li><a href="#">DONATE</a></li>
    </ul>
</nav>

And this:

nav {
    text-align: center;
}

nav ul {
    padding: 0 0 30px 0;
    list-style:none;
    text-align:left;
    white-space: nowrap;
}

nav li {
    vertical-align: text-top;
    display: inline-block;
    margin: 0 -3px 0 0;
}

nav a {
    padding: 15px 20px;
    text-decoration: none;
    font-family: Arial;
    font-size: 24px;
    color: #000;
    border-right: 1px solid #f37f43;
}

nav a:hover {
    text-decoration: underline;
}

#logo a {
    border: none;
}

#logo a:hover {
    text-decoration: none;
}

#logo img {
    width: 142px;
    height: 159px;
    padding-right: 20px;
}

Here's a jsfiddle - should look the same (apart from less margin, but that's because I remove the <body> tag from your code; jsfiddle add that itself; you shouldn't do it yourself)

\$\endgroup\$
  • \$\begingroup\$ Wow this looks great! So I updated the jsfiddle that you included in your answer. I added classes for the navigation links, not the logo, so that the text (and borders) have the same space from the top as the logo does. Is that an acceptable use of classes do you think? \$\endgroup\$ – Justin May 27 '14 at 4:15
  • \$\begingroup\$ @Justin It, again, sounds like you're adding classes to "the rule" rather than "the exception". I.e. if you want all-but-one of those links to behave a certain way, then first make all the links behave like that, and only break the mold for the named (i.e. class'ed or ID'ed) element/elements that should behave differently. That what the code above already does: All nav a elements have a border, but #logo a elements just has a rule that cancels the border. If you want to mess around with margins (or anything else), follow that example. No need for new classes or changes in the HTML \$\endgroup\$ – Flambino May 27 '14 at 8:21

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.