3
\$\begingroup\$

I am rebuilding the front-end for an application that I am building and have the content laid out the way I need.

I would appreciate any suggestions on how to improve the structure, specifically the header section) with web standards in mind.

You can view the Fiddle here. - edit please ignore the non loading icons in the navigation on jsfiddle.

/* http://meyerweb.com/eric/tools/css/reset/
   v2.0 | 20110126
   License: none (public domain)
*/

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
del, dfn, em, img, ins, kbd, q, s, samp,
small, strike, strong, sub, sup, tt, var,
b, u, i, center,
dl, dt, dd, ol, ul, li,
fieldset, form, label, legend,
table, caption, tbody, tfoot, thead, tr, th, td,
article, aside, canvas, details, embed,
figure, figcaption, footer, header, hgroup,
menu, nav, output, ruby, section, summary,
time, mark, audio, video {
    margin: 0;
    padding: 0;
    border: 0;
    font-size: 100%;
    font: inherit;
    vertical-align: baseline;
}
/* HTML5 display-role reset for older browsers */
article, aside, details, figcaption, figure,
footer, header, hgroup, menu, nav, section {
    display: block;
}
body {
    line-height: 1;
}
ol, ul {
    list-style: none;
}
blockquote, q {
    quotes: none;
}
blockquote:before, blockquote:after,
q:before, q:after {
    content: '';
    content: none;
}
table {
    border-collapse: collapse;
    border-spacing: 0;
}

/* Some CSS Setup - nothing to do with flexbox */
html                                 { box-sizing: border-box; }
*, *:before, *:after { box-sizing: inherit; }
body                                 { font-family: 'Source Sans Pro', sans-serif; font-size: 1.5em; font-weight: 100; margin: 0; }

body {
    background: #FAFAFA;
}

    header {
        background: white;
    }

        header h1 {
            font-weight: 400;
            padding: 1em;
            background: #5E35B1;
            color: white;
        }

        #h-wrapper {
            display: flex;
            justify-content: space-between;
        }

        nav {
            display: flex;
        }

            nav a {
                display: block;
                padding: 1em;
                border-left: 1px solid lightgray;
                color: black;
            }

    img {
        width: 100%;
    }

.user-functions,
.user-settings,
.search {
    display: none;
    width: 100%;
    background: #FAFAFA;
    padding: 1em;
}

    .user-functions.open,
    .user-settings.open,
    .search.open {
        display: block;
    }

        .user-functions ul li,
        .user-settings ul li {
            text-align: right;
        }

        .user-functions ul li a,
        .user-settings ul li a {
            color: red;
            text-decoration: none;
            text-transform: uppercase;
            font-size: .65em;
            display: inline-block;
            background: #FFF;
            padding: .5em;
            margin: .5em 0;
        }

        .search input {
            display: block;
            width: 100%;
        }

        .search button {
            display: inline-block;
            float: right;
        }

#top-rated {
    background: #7E57C2;
    padding: .5em 1em;
    display: flex;
    justify-content: space-between;
}
    span.top-posts,
    span.top-builds {
        color: white;
        font-size: .65em;
    }

    .top-posts-toggle,
    .top-builds-toggle {
        background: #000;
        color: white;
        padding: 1em;
        display: none;
    }
        .top-posts-toggle.open,
        .top-builds-toggle.open {
            display: block;
        }

            .top-posts-toggle ul li a,
            .top-builds-toggle ul li a {
                color: #ECEFF1;
                text-decoration: none;
                text-transform: uppercase;
                font-size: .65em;
                display: inline-block;
                background: #455A64;
                padding: .5em;
                margin: .5em 0;
            }

#container {
    width: 100%;
}

    .post,
    .comments {
        margin: .5em;
        padding: .5em;
        background: white;
    }

    .post h1 a {
        text-decoration: none;
        display: block;
        margin-bottom: .25em;
    }

    .post ul {
        display: flex;
        justify-content: space-between;
        margin-bottom: .25em;
    }

        .post ul li a {
            text-decoration: none;
            font-size: .75em;
            background: #80CBC4;
            padding: .35em;
            display: block;
            color: white;
        }

    .post p,
    .comment p {
        font-size: .75em;
        line-height: 1.15em;
    }

    .comment {
        display: flex;
        justify-content: space-between;
        border-bottom: #E0E0E0 1px solid;
        margin-bottom: .5em;
        padding-bottom: .5em;
    }

    .comment:last-child {
        border: 0;
        margin: 0;
        padding: 0;
    }

        .comment .avatar {
            flex-basis: 15%;
            margin-right: .5em;
        }

        .comment div {
            flex-basis: 85%;
        }

        .comment p a {
            text-decoration: none;
            margin-bottom: .5em;
        }
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>BS</title>
  <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/font-awesome/4.4.0/css/font-awesome.min.css">
  <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.4/jquery.min.js"></script>
  <link rel="stylesheet" href="assets/css/style.css">
  <link href='https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,300' rel='stylesheet' type='text/css'>
</head>
<body>
  <header>
    <div id="h-wrapper">
      <h1>BS</h1>
      <nav>
        <a href="#" class="toggleNav" data-target-id="user-functions"><i class="fa fa-plus"></i></a>
        <a href="#" class="toggleNav" data-target-id="user-settings"><i class="fa fa-cog"></i></a>
        <a href="#" class="toggleNav" data-target-id="search"><i class="fa fa-search"></i></a>
      </nav>
    </div>

    <div id="user-functions" class="user-functions">
      <ul>
        <li><a href="#">Create Post</a></li>
        <li><a href="#">Create Build</a></li>
      </ul>
    </div>

    <div id="user-settings" class="user-settings">
      <ul>
        <li><a href="#">Edit My Account</a></li>
        <li><a href="#">Profile</a></li>
      </ul>
    </div>

    <div id="search" class="search">
      <form>
        <input type="text" placeholder="search">
        <button>search</button>
      </form>
    </div>

  </header>

  <div id="top-rated">
    <span class="toggleNav top-posts" data-target-id="top-posts">Top Posts</span>

    <span class="toggleNav top-builds" data-target-id="top-builds">Top Builds</span>
  </div>

  <div id="top-posts" class="top-posts-toggle">
    <ul>
      <li><a href="#">This is a top post...</a></li>
      <li><a href="#">This is another top post...</a></li>
      <li><a href="#">How did this become so...</a></li>
      <li><a href="#">Maybe we can do this...</a></li>
      <li><a href="#">This is a top post...</a></li>
    </ul>
  </div>

  <div id="top-builds" class="top-builds-toggle">
    <ul>
      <li><a href="#">This is a top build...</a></li>
      <li><a href="#">This is another top build...</a></li>
      <li><a href="#">How did this become so...</a></li>
      <li><a href="#">Maybe we can do this...</a></li>
      <li><a href="#">This is a top build...</a></li>
    </ul>
  </div>

  <div id="container">
    <article class="post">
      <img src="http://www.history-of-cars.com/images/ford-usa/1931-model-a-5window-coupe-old-style-112.jpg" alt="some image" />
      <h1><a href="#">1931 Ford 5 Window Coupe</a></h1>

      <ul>
        <li><a href="#"><i class="fa fa-thumbs-o-up"></i> 23 Likes</a></li>
        <li><a href="#"><i class="fa fa-heart-o"></i> 44 Favorites</a></li>
      </ul>

      <p>Here is a nice ride by Camsnappers Hot Rods & Customs!</p>
    </article>

    <article class="comments">
      <div class="comment">
        <div class="avatar">
          <img src="https://upload.wikimedia.org/wikipedia/commons/c/cd/Joni_Mitchell_(1975).png" alt="joni mitchell avatar" />
        </div>

        <div>
          <p><a href="#">nucky15</a></p>
          <p>What a great pic!</p>
        </div>
      </div>

      <div class="comment">
        <div class="avatar">
          <img src="https://upload.wikimedia.org/wikipedia/commons/c/cd/Joni_Mitchell_(1975).png" alt="joni mitchell avatar" />
        </div>

        <div>
          <p><a href="#">nucky15</a></p>
          <p>Please show us more of your work!</p>
        </div>
      </div>

      <div class="comment">
        <div class="avatar">
          <img src="https://upload.wikimedia.org/wikipedia/commons/c/cd/Joni_Mitchell_(1975).png" alt="joni mitchell avatar" />
        </div>

        <div>
          <p><a href="#">nucky15</a></p>
          <p>Walking on broken glass. The sun is still shining in the big blue sky.</p>
        </div>
      </div>
    </article>
  </div>


<script>
var currentOpenId;

// apply handler to all elements with the data-target-id attribute
$('[data-target-id]').click(function(e) {
  // prevent default action
  e.preventDefault();

  // get target id
  var id = $(this).data('target-id');

  // close previous menu
  if (currentOpenId) {
    $("#"+currentOpenId).removeClass('open');
  }

  // open different menu
  if (currentOpenId !== id) {
    $("#"+id).addClass('open');
    currentOpenId = id;
  }

  // unset id because no menu is open anymore
  else {
    currentOpenId = null;
  }
});

</script>
</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. "Apologies, I could not get FontAwesome to work in the Fiddle, so there are blank squares." This sounds like you have a problem in your code. If you want that problem fixed, Code Review is not the place. We review only working code. There is one exception though: if there is an edge case, or a minor bug, that you want the reviewers to ignore in their review, then it is fine on Code Review. \$\endgroup\$ – TheCoffeeCup Dec 8 '15 at 23:02
  • \$\begingroup\$ @TheCoffeeCup Thank you for the welcome. The code does work, if I had it hosted even. It seems when including the CDN on jsfiddle the icons just do not appear for some reason. I hope this is not against the rules but I am not looking for a solution to that. \$\endgroup\$ – Jonathan Musso Dec 8 '15 at 23:05
  • \$\begingroup\$ You are using FontAwesome 4.4. You should be using 4.5. \$\endgroup\$ – Ismael Miguel Dec 8 '15 at 23:08
2
\$\begingroup\$

Responsive

  1. This is really nice with em and percentages being used effectively - get your head around that and media queries and that's responsiveness for screen sizes.
  2. Media Queries are your friend and can allow you to re-organise things on smaller screens or different aspect ratios.
  3. Responsive image load is a nice idea where you have several images with different widths and maintained aspect ratios, you then load them progressively wider and wider until the last one that fits inside the viewport. This means your user never loads an unnecessarily large image and doesn't have to wait for a huge image to load on a slow connection.
  4. Remember that responsive design is a consideration of three main things: screen-size; bandwidth; and touchscreen (a finger is very different interface from a mouse pointer).

CSS

  1. Don't use IDs - there's no need, a class can achieve anything an ID can without being restricted to one use on a page or imposing sledge-hammer specificity. More here
  2. Don't use html entities in css, such as this .comment div - there are three reasons: firstly, you'll end up adding another tag into the parent and it will get unexpected styles; secondly, you won't be able to re-use the css without copying the HTML structure too; and thirdly, it introduces unnecessary specificity. Adding a class to the target is better (div in the one I picked out) and styling that directly is best (this applies to selectors in JS too)
  3. You can put box-sizing: border-box; directly on the * selector rather than using html and inherit.
  4. State classes can be identified by prefixing with is- so you'd have is-open. You can put these at the end of a style sheet and add them to anything without qualifying them (rather than .user-functions.open). You do need to be careful with specificity for this to work well.
  5. Don't use overqualified tags, so span.top-posts is not good. .top-posts is better as it's re-usable without being put on a <span> and won't break as easily when the mark-up changes.

HTML

  1. <article> tags are not quite right. I'd use an <article> tag to cover the text and the comments, with <section>s within that, including one <section> for the comments and each comment as an <article>. The spec here has a couple of examples of marking up blog posts. You may also want to consider a <header> and <footer> tag in the article.
  2. Add some aria-roles. For example, <nav role="navigation"> - this is good for any mechanical interpretation of your content, most crucially accessibility tools like screen readers. role="main" can go on your main article. You might think using a <main> tag would be good here, but given the structure of article body with comments, a containing article tag is better. Here's a nice cheatsheet and here's the spec.
  3. I'm guessing toggleNav is a js-hook. It's nice to differentiate them and to separate them from the css hooks, however, I prefer the js- prefix for this rather than CamelCase. This is preference really but is also common practice: all class attributes are snake-cased.
  4. The list of links in the <nav> element should be organised as an unordered list like all the others contained within the <nav> tag. This is standard practice. You could also drop the <nav> and use the role described in point 2 on the <ul> element (incidentally, if you make this change, you'll notice why using html elements in css can be buggy!). This is a bit flexible, the key thing is probably to organise the links in a proper HTML list.

JavaScript

  1. Nice to see add and remove class being used with a state class in JS.
  2. You've passed in an anonymous function to the click() method. You could use a re-usable function variable, like this:

    var clickHandler = function() { /* commands */ }
    $('.js-element').click(clickHandler);

There are several good reasons for this, including that it's nicely named and re-usable. This article has more detail.

\$\endgroup\$

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.