Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm currently looking to try learn CSS properly and I'm looking to recreate the following image using CSS:

enter image description here

Now I've managed to recreate the image (sort of) what I am looking to know though is based on my code what am I doing wrong/what would be the best practice. Many solutions fix a problem I want to know if my solution is reasonable.

<div style="width:350px;height:380px;position:relative;box-shadow: 0 1px 4px rgba(0, 0, 0, 0.5);border-color: #ddd;">
<div style="width:100%;height:70px;background-color:red">
</div>

<div style="height:180px;width:350px;background-color:gray;">
</div>

<div style="background-color:yellow;bottom:0px;position:absolute;width:350px;height:130px;">

        <p style="margin-top:0px;position: absolute;top: 16px;left: 16px;">is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's</p>

        <div style="width:25px;height:25px; background-color:blue;display:inline-block;position: absolute;bottom: 23.5px;left: 16px;"></div>
        <div style="width:25px;height:25px; background-color:blue;display:inline-block;position: absolute;bottom: 23.5px;left: 57px;"></div>
        <div style="width:90px;height:40px; background-color:pink;display:inline-block;position: absolute;right: 16px;bottom:16px;">Testing</div>

</div>

Here is the CSS image:

enter image description here

share|improve this question
3  
just one point: It's bad practice to use inline css, use class/id instead and link to an external css file (the reasons are among others: better performance (cache), better maintainability (separation of markup and style) and avoiding duplicate code) –  tim Aug 20 at 19:52

2 Answers 2

Here is my CSS

Well, no, actually. That's your HTML - it just happens to have some style attributes. Which it shouldn't have.

So step one, separate CSS and HTML. Use a <style> element in the <head> element, or better yet, include the CSS from a separate file. Don't inline your styling.

But for experiment purposes, you could try re-making your code as a jsfiddle or jsbin (both for your own sake, and to make it easier for reviewers later). Put HTML in the HTML panel, and CSS in the CSS panel, and play around with it.

And you should be using more semantic (i.e. meaningful) markup. You have a lot of div elements, but a div is a purposefully meaningless element used mostly for grouping other, more specific, elements.

The point is that when just reading the HTML, I can't tell what it represents. It could be anything since it's almost all div elements. Yet in the picture I see things that should probably be links or buttons, headings or spans, and maybe forms and images.

Here's an analogy to illustrate the idea of "semantic markup": Imagine for a moment that you're writing a book: the HTML elements form your outline. Right now, that outline would say:

  • Text
  • Text
  • Text
  • Text

Ok, sure, a book consists of text, but that's not a very useful outline.

A more useful book outline would say something like

  • Title
  • Introduction
  • Main body (likely divided into chapters or sections)
  • Summary and conclusion

That's what HTML is meant to do: Mark up information so as to reveal and codify its structure. Hence, the tags you use matter a great deal. And it makes it easier to apply CSS, since you've now grouped content in a logical manner. You can style things based on what they are; headings, paragraphs, images, etc.. You want all your links to be red? Just change the style for a elements, and you're done. The HTML stays the same, since the links are still links - the CSS just makes them look different, but doesn't (shouldn't) change their meaning.

That said, there of course aren't pre-existing elements for everything you can think of, and often you want some headings to look one way, and other to look a different way, even though they're all headings. That's when class names and sometimes IDs come in, since they allow you to further specify things.

When I look at that first image, I see a structure (outline) like

div.dialog
  header
    img.thumbnail
    h1
    h2 (update: an h2 isn't really correct here, see comments)

  div.body (maybe it's actually an image?)

  footer
    p
    div.actions
      a.favorite
      a.share
    form
      button
      button

I've given the containing element a dialog class, because it looks sort of like a dialog box to me. And yes, it's a div because there's no HTML element that captures the idea of a "dialog". That's when div comes in handy, since you can apply your own meaning (semantics) to it using an ID and/or a class name.

But it might not be a dialog box; it might be a cell in a grid view. In that case the class name should reflect that and be called grid-cell or something (update: actually, no it probably shouldn't - see comments). And perhaps the element should be an article. Again, pick an element and, if necessary, an ID and/or class name to describe what something is (not how it looks, or how it behaves).

Now, I'm not saying that the structure above is correct (as you can see, a few things have already been brought up in the comments), but that's how it mapped out in my head. If I were to start actually coding it, I'd probably end up changing a bunch of things.

For instance, footer probably isn't quite the right element to use, since it really contains more important stuff. Yet is it the "body" then? I don't know, but you've got to start somewhere, and then iterate, iterate until the CSS and the HTML each look clean and semantic separately, and together produce the desired result.

I know I haven't actually tackled the styling you've written, but that's because I find that less important than getting the overall approach right first.

share|improve this answer
    
+1. This is a really good answer. –  You Aug 20 at 23:42
1  
I'm going to have to disagree here on the idea that grid-cell describes what the something is. Grids are a visual concept, not a semantic one. If the design changes and the items are no longer in a grid, will grid-cell make any sense? If not, then it is a poor choice in names. –  cimmanon Aug 21 at 1:33
1  
Also, the use of the h2 element for a subheading is semantically incorrect‌​. The div.actions is a list of actions, so it should be a list instead. –  cimmanon Aug 21 at 1:38
    
@cimmanon Fair points, I'll put some notes in there - but as I said in the answer, this was a first, early pass. I did write the caveats "I'm not saying that the structure above is correct" and "I'd probably end up changing a bunch of things". –  Flambino Aug 21 at 7:28
    
There is actually <dialog> element in HTML 5, but browser support is very patchy. Also, the OP's image doesn't look like dialog box, more like the boxes that would fit into a G+ stream. Also, I would have used ul for div.actions. –  Lie Ryan Aug 22 at 11:20

By using inline styles, you're creating 3 problems for yourself:

  • Maintenance nightmare
  • Making your markup cost more to download (an external stylesheet is downloaded once and cached)
  • Making it difficult to discern what the elements are meant to mean

As far as the markup itself goes, never use empty elements unless you're populating them later via JS. Always make sure the document makes sense when CSS is disabled (bots don't see CSS at all).

I've attempted to clean up your markup and give meaning to the elements:

<div class="container">
  <header>
    <h1>Title</h1>
    <p>Subhead</p>
  </header>

  <div class="foo">
    <!-- not sure what this is meant to be, there's no clues in the original -->
  </div>

  <p>s simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's</p>

  <footer>
    <ul class="actions">
      <li class="favorite">F</li>
      <li class="next">N</li>
    </ul>

    <ul class="call-to-action">
      <li>Action 1</li>
      <li>Action 2</li>
    </ul>
  </footer>
</div>

Now that that's cleaned up a bit, we can go about fixing the CSS.

You're using absolute positioning when you don't need to be. It should only be used when you need to move elements out of the flow of the document (eg. drop menu or sticky footer). You'll find that using absolute positioning for layout will bite you in the ass later when content starts overflowing from your containers or you can't scroll to the end.

Don't want your elements sticking to the edge of the parent elements? Use margins or paddings, not positioning.

When you absolutely position something, you don't need to change the display property (unless it was none).

If your containers contain text, avoid using fixed units like px. By using units that will scale with your text, such as em, your containers will feel appropriately sized no matter what font size is used (especially when some users will increase the default font-size for accessibility reasons).

.container {
  max-width: 20em;
  box-shadow: 0 1px 4px rgba(0, 0, 0, 0.5);
  border-color: #ddd;
  background-color: yellow;
}
.container header {
  background-color: red;
  padding: 1em;
}
.container header h1 {
  margin-top: 0;
}
.container header p {
  margin-bottom: 0;
}
.container .foo {
  height: 180px;
  background-color: gray;
}
.container > p, .container footer {
  padding: 0 1em;
}
.container footer {
  display: table;
  width: 100%;
  box-sizing: border-box;
  padding-bottom: 1em;
}
.container footer ul {
  display: table-cell;
  width: 50%;
  padding-left: 0;
}
.container footer li {
  display: inline-block;
}

Demo: http://codepen.io/cimmanon/pen/peCdb

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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