HiveBrain v1.2.0
Get Started
← Back to all entries
patternhtmlMinor

Correct way to center logo and navigation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewcssstackoverflowbeginnerhtml5html
navigationwaylogocentercorrectand

Problem

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:


    
        
            
                
            
        
        WHO WE ARE
        WHAT WE DO
        EXPLORE
        DONATE


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;
}

Solution

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 ` 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 element, and it's given that it's a link because it's an 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
  • containing the logo an ID - simply call it logo. This lets you do two things:



  • You can style all the list items the same basic way, and only do something different for #logo



  • You can get rid of the ID on the element, since you can instead reference it as #logo img



I get this:


    
        
            
        

        WHO WE ARE
        WHAT WE DO
        EXPLORE
        DONATE


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 ` tag from your code; jsfiddle add that itself; you shouldn't do it yourself)

Code Snippets

#nav_links li a
#nav_links            << high-level
#nav_links li
#nav_links li a:hover << low-level
#nav_links            << high-level
#nav_links li
#nav_links li a
#nav_links li a:hover << low-level
<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>
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;
}

Context

StackExchange Code Review Q#51787, answer score: 4

Revisions (0)

No revisions yet.