Testimonials Grid Challenge Solution

Community feedback

• 

  Huy Phan•2,300

  @huyphan2210Posted about 1 month ago

  Hi, @Crtykwod

  I checked out your solution and I have some thoughts:

  • Good job completing the challenge in under 30 minutes! However, speed can compromise accuracy. If you take your time, you could improve certain aspects. For instance, adding more media queries would prevent horizontal scrolling between 1024px and 1560px viewports to ensure the grid displays properly.
  • Your HTML structure could also benefit from refinement. You've used semantic tags in many places, so why not replace the remaining divs with more appropriate tags? They stand out against the rest of the semantic structure.

  I think the goal of Frontend Mentor is to hone your skills. Instead of rushing, focus on how you can refine and improve each challenge for better learning.

  Hope this helps!

  1

  ---

  

  P

  Carlos Samuel•100

  @CrtykwodPosted about 1 month ago

  Hello, @huyphan2210! Thanks for your feedback!

  First of all, when you declare a new section or article, u need to give a heading to it, like in a newspaper, all articles have a heading on them. I used divs only for styling purposes, so no need for semantic tags.

  Also, the rush really was a problem, as I did a bad code review :p

  I usually make my grid layouts with fixed values ​​for column and row and then change them to minmax(). But I forgot to change the value, this is what gave the desing a horizontal scroll. So I fixed it and took it as an opportunity to create an intermediate layout for tablets.

  Again, thank you for the feedback, it helped me notice my lack of attention, thx ^-^

  1

  

  Huy Phan•2,300

  @huyphan2210Posted about 1 month ago

  @Crtykwod

  I suggest replacing those divs with an hgroup element since they seem to be used for headings. Additionally, you might want to wrap the image in a figure or picture tag.

  You could also consider how the grid layout should adapt on tablet devices. While the challenge might not include a tablet design, creating one could help prevent horizontal scrolling on desktop. Just a thought!

  Thanks for your response.

  1

  

  P

  Carlos Samuel•100

  @CrtykwodPosted about 1 month ago

  @huyphan2210,

  • Well, those divs are not used for headings, to be honest, i used <h2> nearly against my will, because they are necessary, but doesn't make much sense for me. Also, the <p> isn't part of the heading, like a subheading, so it can't be inside a <hgroup>.

  • About <figure>, according to MDN, usually a <figure> is an image, illustration, diagram, code snippet, etc., that is referenced in the main flow of a document, but that can be moved to another part of the document or to an appendix without affecting the main flow. The photo is part of the flow, it needs to be there, so it's not a <figure>.

  • And the <picture> tag isn't even semantic, it's just used to set different sources to a image container and there's no need for more than one source for a fixed size profile picture.

  • Usually, i do consider tablet designs, i was just trying to speedrun this challenge, so i didn't make it XD. But that is indeed a good advice!

  I didn't even know about <hgroup>, and it's definitely a very useful tag, so thanks for presenting me it!

  1