3
\$\begingroup\$

I created this testimonial section as a prototype.

testimonial concept

HTML

<div class="testimonials">
  <blockquote>
    <p>This was pretty good.</p>
    <cite>
      <span class="author">– Bobby,</span> Jersey City, NJ
    </cite>
  </blockquote>

  <blockquote>
    <p>This is pizza is the most bearable thing I ever had in Joisey. But it still sucks, just like everyone from there.</p>
    <cite>
      <span class="author">– Vinny,</span> New York, NY
    </cite>
  </blockquote>

  <blockquote>
    <p>I really savored the smoky undertones of the imported sausage. The cheese had a wonderful texture. It paired well with my pinot noir in a dance of sensations.</p>
    <cite>
      <span class="author">– Alex,</span> San Francisco, CA
    </cite>
  </blockquote>
</div>

CSS

.testimonials {
  width: 720px;
  max-width: 96%;
  margin: 0 auto;
}
.testimonials blockquote {
  background-color: #fff;
  border-left: 4px #61acca solid; 
  font-size: 21px;
  line-height: 1.6;
}

.testimonials blockquote {
  padding: 10px 20px; 
}

.testimonials blockquote {
  background-image: url('http://shared-assets.s3.amazonaws.com/codepen/img/elements/quotes/quote-999.jpg');
  background-repeat:no-repeat;
  background-size: 33px 45px;
  background-position: 10px 5px;
}

.testimonials cite {
  font-size: 0.7em;
  font-style: normal;
}
.author {
  font-weight: bold;
  font-size: 1.3em;
  margin: 0 6px 0 36px;
}
p { margin: 0 0 0 36px; }
\$\endgroup\$
  • 2
    \$\begingroup\$ What is your question about CSS inheritance? \$\endgroup\$ – Jukka K. Korpela May 10 '14 at 6:14
  • \$\begingroup\$ @JukkaK.Korpela If I properly set up the rules to inherit as opposed to just adding classes to specific elements. \$\endgroup\$ – JGallardo May 12 '14 at 17:32
  • \$\begingroup\$ “If...”, then what? And the code in the question has no inherit settings. \$\endgroup\$ – Jukka K. Korpela May 12 '14 at 17:36
  • \$\begingroup\$ @JukkaK.Korpela It does not seem that you understand what inheritance is. Some rules are inherited by default. Please see: css specificity and inheritance \$\endgroup\$ – JGallardo May 12 '14 at 17:47
  • \$\begingroup\$ No, rules are never inherited. Properties may be inherited. But you still have not said what is your question about CSS inheritance. \$\endgroup\$ – Jukka K. Korpela May 12 '14 at 18:01
2
\$\begingroup\$

Not bad at all. I wish we could see more of this kind of code online! However, allow me to make a few suggestions.

HTML:

  • Technically this is a list of testimonials. So I would suggest using a unordered list.
  • Where you use the cite tag, I would use the footer or address tag in stead.
  • I would place your author inside a cite element
  • The - in font of the name can be considered styling, not content, so I would use a :before pseudo element inside the css to add it

That means the HTML would result in something like this:

<ul class="testimonials">
  <li>
    <blockquote>
      <p>This was pretty good.</p>
      <footer>
        <cite class="author">Bobby,</cite> Jersey City, NJ
      </footer>
    </blockquote>
  </li>
  ...
</ul>

Note that the cite / footer / address thing is open for discussion. Your way should be fine as well. It is just the way I would do it...

CSS:

  • As said by @JosephTheDreamer you should indeed keep your font-size units consistent. Personally I would choose px however. Just because I find them easier to work with. And the days we needed em to enable browsers to resize fonts are luckily behind us. Eather should be fine though, just stick with you choice.
  • Why do you repeat your .testimomials blockquote selector 3 times? Just keep them in one block. You could add whitespace for good readability, but repeating the selector will just make it harder to maintain your code and even slow down the processing.
  • Rather then adding a margin-left of 36px to you p and .author, would would add an extra 36px of padding-left to your wrapping blockquote. This way they are easier to maintain, and you eliminate the need for you p selector.
  • Namespacing your css is indeed a good idea. Just be careful not to overdo it. The longer your selector gets, the slower and heavier it becomes. Also you are in danger of writing a lot of styles multiple times. In the case of your blockquotes i.e. one could wonder if there are going to be other blockquotes on your site that are going to look entirely different. Chances are that you want a uniform style for your website, and that other blockquotes will have to look almost the same. I would suggest putting all styles that apply to all blockquotes in a simple blockquote selector. Anything that is different then the 'normal' style can be put inside the namespaced selector you are using.
  • There should be no need to set your font-style to normal on your cite element. Defenitly if you are using some sort of reset or normalize css (which you should). This means you can ommit that rule.
  • p { margin: 0 0 0 36px; } Please don't, and I mean don't, put your selector and your properties on the same line! It looks awfull and makes it harder to maintain your code, or find something quickly. Use a minimizer for your production css, but keep your selectors and properties each on their own line in your dev version!
  • I actually disagree with @JosephTheDreamer on resizing your background image. I quite often bring my images to halve their size to make them look nice and crisp on hegh resolution (retina) screens. Just be careful not to make your image much bigger then that, as it will just increase the load time. I would even go for a icon font inside a :before pseudo element in the case of that quotation mark. It is probably smaller, renders sharp on all screens, and can be scaled and colored as you want trough css, as it is in fact a vector. Something like icomoon can make this very easy for you.

That's about it I think. And again, note that I have been nitpicking here. This is codereview after all. Your code is quite fine as it is, and I would probably hire any developer that writes code of this quality.

\$\endgroup\$
  • \$\begingroup\$ Good response, I'll also add you could go even further with using Microdata/RDFa for authors/location as well and add a <time datetime=""> for when the testimonial occurred. \$\endgroup\$ – darcher Jun 25 '14 at 22:44
2
\$\begingroup\$

Everything looks clean. However, I found some things though:

  • font-size - Make the units consistent. You use px and em. I suggest you go em all the way.

  • background-size - Well, it exists for that reason, but it would be better if you actually sized the image in those dimensions rather than having them resized in CSS.

  • You "namespaced" everything by prefixing .testimonials. That's being a good developer. You're avoiding your styles from clobbering other elements. However, you forgot to do that for .author and p.

\$\endgroup\$
  • \$\begingroup\$ No suggestion of the rem value with px fallbacks? ;) \$\endgroup\$ – darcher Jun 25 '14 at 22:47

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.