Social-proof-section

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Angel!

  🎉 Congratulations on finishing this challenge! I have some feedback on this solution:

  • Accessibility
    • All page content should live inside a landmark (header, main, and footer). In this case, you can wrap all the page content with main tag.
    • For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only, except the photos.
    • For the testimonial photos, I recommend using their names as the alternative text.
    • To learn more about how to make images accessible, you read the tutorial from W3 WAI about images and the ultimate guide for alternative text by Axess Lab.
    • Heading tags should always be in order. Also, there's no need to make the reviewer's name as a heading, since the content below it, is too small.
    • You can make the stats to be a ul instead of a section and wrap each list item with li. section and article always needs a heading as a label.
    • Wrap the testimonial with blockquote for semantic HTML.

  <blockquote>
    <p class="card__text">
      "We needed the same printed design as the one we had ordered a week prior.
      Not only did they find the original order, but we also received it in time.
      Excellent!"
    </p>
  </blockquote>
  

  • Use rem or sometimes em unit instead of px. Using px will not allow the users to control the size of the page based on their needs.
  • Styling
    • For the background images, I would recommend using background properties on the body element.
    • I would recommend making the body element as a flexbox container to make the page content perfectly in the middle of the page.

  body {
    display: flex;
    align-items: center;
    justify-content: center;
    min-height: 100vh;
  }
  

  • There's no need to set a min-width: 100vw since by default the body element already have 100% width.
  • I would recommend using the mobile-first approach when you write the styling. It often makes me write less code. Also, you can avoid having complex media query, especially on this project. So, all you have is the @media screen and (min-width: 80em). (80em is equal to 1280px).
  • Also, in addition to @media query, I recommend using em unit instead of px. You can read this article about what unit you should use on @media query.

  That's it! Hopefully, this is helpful!

  Marked as helpful

  0
• 

  P

  Remus D. Buhaianu•3,125

  @MiculinoPosted almost 3 years ago

  Hey @AngelG-JAPY

  Good job on completing the challenge! It looks really nice! I have a few suggestions for you that I hope will be useful based on your final solution:

  • Your container should be horizontally centered.

  • The two background images from the original design are missing. Try to add those into your final solution as well

  • The responsive design for mobile screen sizes requires a bit of work. Try to make the appropriate adjustments for that resolution based on the original design

  Marked as helpful

  0