Responsive testimonial section using flexbox and grid

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one and also thanks for letting me know to review this one^^. Layout is great in desktop as well for the mobile state. A suggestions though for the responsive state is to maybe add a 2x2 layout before transitioning to mobile state. Currently, at 744px which still shows the desktop state, you could see that the layout is squished, that is having a 2x2 would be really great.

  Actually, seeing it the layout for Kira is not really overlapping the content, have you already fixed it?

  Just some suggestions for the site would be:

  • Having a :hover effect on an un-interactive element is not really great because you only want to animate an element if it is bound to be interacted with. But if you insist on this one, it would be better if the animation is just subtle.
  • There should always be an h1 on a page. Since there are no text-content that are visible that could be h1, you will make the h1 screen-reader only text. Meaning this will be hidden for sighted users and only be visible for screen-reader users, search about sr-only stylings and see how it is used. The h1 text should describe what is the main content is all about, this h1 would be placed as the first text-content inside the main element.Have a look at Grace's solution this challenge, inspect the layout and see the usage of h1 as well the stylings applied to it.
  • When using heading tag, make sure you aren't skipping a level. If you use h3 make sure that h1, h2 are present "before" it.
  • The person's position is not really suited to be a heading tag, use p tag on it. Just the name of the person is the one that should be a heading tag, because they are the highlight of the section and it is already known that the section would contain the person's testimonial since this is a testimonial section.
  • The testimonial bold text is not really a heading tag, if you read it, it is just part of the text-below. The text doesn't really convey anything that the section would contain.

  Aside from those, the site looks great. Haven't seen what you mentioned by overlapping.

  Marked as helpful

  1

  ---

  

  Michael•100

  @mikeyxxPosted about 3 years ago

  @pikamart Thank you for these pointers. I will make all of the changes you suggested. Here is a screenshot of the overlap I told you about: https://github.com/mikeyxx/testimonials-grid-section/blob/master/README.md

  The overlap on the Kira grid is adding and extra space to the row gap. Please check it out.

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @mikeyxx Hey, so I found out about it.

  The margin-block-end: 1.3rem; on the .testimonial selector is causing that bug, try removing it and the desktop layout will be fixed. But doing that, going into mobile there are no spacing, so on mobile add a gap property on the .container. ^^

  Marked as helpful

  1

  

  Michael•100

  @mikeyxxPosted about 3 years ago

  @pikamart You are amazing, I love your feedbacks. I have implemented all the pointers you raised and now my solution looks really good. Also thank you for pointing me to Grace's solution, looking through her code was helpful. I will now be moving on to challenges that include JS (I'm terrified lol) as I'm still trying to perfect my use of DOM. I hope I can still reach out.

  Thank you :)

  1
• 

  Michael•100

  @mikeyxxPosted about 3 years ago

  @pikamart Hello Raymart Pamplona, please help review my project. Only issue I have is adjusting the grid for Kira whittle, its overlapping the others. I have tried severally to fix the issue but I haven't been able to. I'd really like your feedback

  0