testimonials-grid-section-main with css grid

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in desktop looks fine, it is just smaller than the original, the responsiveness could be better since right now, if you lower down the screen's width, the elements are squished. The mobile layout however looks great.

  You already got a great feedback, just going to add some as well:

  • Always have an h1 element on a webpage. Since there is no suitable visible h1 on this, you would make the h1 an sr-only element, which the text of the h1 describes what is the main content is all about. Take a look at Grace's solution on this one inspect the layout and see how she uses the h1 on it, also copy the styling that is applied on the h1, you would need that a lot.
  • Do not use vh unit on the height property. This limits the height of an element, based only to the remaining viewport/screen's height. On your .grid-container selector, it is better to remove that styling on the height property.
  • Name for each person could use a heading tag.
  • The bold text on the testimonial could be better if it was using just a regular p tag since it does look like it is just regular text. Using heading tag on the person's name gives it already a defined structure.
  • Lastly, just maybe making the site more as responsive as you can.

  Aside from those, great work again on this one

  Marked as helpful

  1

  ---

  

  Zybartas•480

  @ZybartasRingysPosted about 3 years ago

  @pikamart

  Hey, big thanks for a nice code review, all topics will help me in my future projects. I did some changes.

  I did this h1 with sr-only class, not perfect but i think it will do the job. Removed vh unit on main grid-container and its working fine. Changed all Name to h3 tag looks good. I did some changes to my responsive design, looks better, but not perfect yet :)

  0
• 

  Muhammad Shajjar•1,100

  @muhammadshajjarPosted about 3 years ago

  Hello Zybarkas! Nice work on this one, here are some suggestions which may help you

  • Make your gird-template-columns to 1fr so they all equally distribute their widths, Using % and auto will cause issues Check this
  • It's good to add breakpoint between 700-800px viewport width, because your layout breaks at that point with grid-template-areas you would easily achieve another layout check out my solution hope it would help you -Try to add max-width to your body or make a container class that wraps your all content, by adding it you can control your content from growing too much on larger viewports
  • You are switching displays, I think don’t need to switch to flex for the mobile design you can achieve that with grid too -To overcome your accessibility issue you would need to add some semantic markers to designate sections of the page as the header, navigation, main content, and footer e.g:


  <main>
  <section> 
  </section>
  </main>
  <footer>
  </footer>
  

  Marked as helpful

  1

  ---

  

  Zybartas•480

  @ZybartasRingysPosted about 3 years ago

  @muhammadshajjar

  Hey, big thanks for a nice code review, all topics will help me in my future projects. I did some changes.

  i have changed my gird-template-columns to 1fr and its working! i have added one more break point, now its looking way much better when i scrolling down to mobile view, also i added max-width of 1440p to my grid container i hope this will help. I switched to display grid with grid areas and mobile layout looks same, also added some semantics.

  0