Nested Grids and Flexbox

Community feedback

• 

  Raymart Pamplona•16,040

  @pikapikamart

  Posted over 3 years ago

  Hey, great work on this one. The desktop layout looks fine, though the h1 is small, the text below it is not using a left-alignment and maybe adding more padding to the body or in a container so that the content won't look like spaced out. The site responds but if you go to at 500px upwards, you will see that the content is being hidden by the screen and creates horizontal scrollbar.

  Here are some other suggestions for the site:

  • Avoid using height: 100% or height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
  • The font-weight: 700 on the h1 could be remove since by default it already uses this.
  • For the .reviews selector, you don't really need to use grid on that one since the it's direct child is already a block element that will wrap on its own row. You just need to use margin on those item to place them properly like on the design.
  • For each .review, using article on them is not the best choice since an article tag usually contains content that is independent and could be redistributed on another page since it can sit on its own. For this one, you could just use div on each.
  • Each of the star icons, you can just use the image path as the value for the background property of each .review selector. Remember that background can contain multiple image, this way you won't have to create multiple img tags.
  • Though, if you insist on using img tags for the star-icons, then make sure to add alt="" and aria-hidden="true" so that screen-reader will skipped that image since it is only a decorative image.
  • For each person's img tag, it would make sense to use their full name as the alt value since it is already present.
  • Also, for each testimonial, you could use this markup below so that it will be easy for screen-reader to traverse it using blockquote:

  <figure>
   <img src="" alt={person name}>
   <blockquote>
     <p> {qoute in here}</p>
   </blockquote>
   <figcaption>
     person name
     <p>
       person role
     </p>
   </figcaption>
  </figure>
  

  Though you just need to use grid on the figure to place each item properly since a figcaption should be the first or last item of a figure element.

  • The verified-buyer should not be using a heading tag since it doesn't really give information on what a section/part of the layout would contain. A p tag on it would be nice.
  • Lastly, addressing the responsiveness issue on the site:>

  Aside from those, great job again on this one.

  00

  Marked as helpful
• 

  Rafael La Buonora•160

  @rlabuonora

  Posted over 3 years ago

  Hi! Thanks a lot for your feedback. I recoded the layout to solve some of the problems you pointed out.

  I focused mainly on the layout because I'm trying to learn that, so I didn't implement the semantic html suggestions. I'll probably revisit it in a few days for those.

  Thanks again!

  10