Testimonials section (mobile first, CSS Grid and Flexbox, SCSS, BEM)

Community feedback

• 

  Szymon Rojek•4,540

  @SzymonRojekPosted almost 4 years ago

  @kamilcodes

  I like your RWD - done well! :D

  I have checked your HTML and RWD, a few tips from me:

  • I'd recommend reading about semantic tags, headings, alt text.
  • remove CSS styles and transfer them to the CSS file;
  • instead of the main-wrapper you can use the main tag (if this div is needed);
  • this is a single page component so you can add the h1 tag, generally tag should be used gradually => you can not skip them;
  • in the "testimonial-card__user-details" you have used spans but name of the person can become the h1, and status graduate can become a paragraph;
  • alt text => don't need to use words like picture or image, photo, icons in the alt text as it's already announced as being an image.

  Greetings :D

  1

  ---

  

  kccodes•125

  @kamilcodesPosted almost 4 years ago

  Hi @szymonRojek! Thanks a lot for your feedback. I tried to approach this challenge as a reusable component, that would be a smaller section of a bigger website in a real life scenario, thats why I left out the h1.

  I like the idea to put the h1 and the p tag in the "testimonial-card__user-details". And great hint regarding the alt text! I will change this.

  I used more semantic html for the testimonial cards: article, h2 and blockquote. And used divs for styling purposes only, since they dont add any additional semantic meaning, thats why I used the .main-wrapper. I used this div, only to center the testimonial-grid-section and the .attribution part - this could be omitted and is only optional. I tried different solutions, but this was the only way I could get the spacing to look "okayish" on big screens XD.

  What do you mean by: remove CSS styles and transfer them to the CSS file?

  1

  

  Szymon Rojek•4,540

  @SzymonRojekPosted almost 4 years ago

  @kamilcodes

  Hi. That’s ok. According to your question => you have got CSS styles (footer) at the top of your code and they are unnecessary in the HTML structure.

  Cheers :D

  1
• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  Hi @kamilcodes, congrats on your first submission! Its really close to the design and great control on medium widths. I think you could've deleted .main-wrapper and still come up with the same outcome, with one less element to maintain. Not sure why it was using grid. If you were trying to center the component, you could just add justify-content: center to the body, since it was already using flex. Hope this helps!

  0

  ---

  

  kccodes•125

  @kamilcodesPosted almost 4 years ago

  Hi @emestabillo, thanks for your feedback! You´re totally right. I could also use flex. I used the .main-wrapper only for styling purposes, to center both the testimonials-container and the .attribution inside the body elemenent and get the spacing right with gap and a little bit of padding-bottom (espacially for the big screen size), so the grid-contianer sits nicely in the center. And I didn´t want to put styling on the body element. Thats why I decided to use grid on the .main-wrapper. I think in a real life scenario, the "attribution class" could be omitted, so the .main-wrapper would be useless. Hope this makes sense. Thanks again!

  0