Frontend-Mentor project: QR code component

Community feedback

• 

  P

  Teodor Jenkler•3,820

  @TedJenklerPosted 2 months ago

  Hi @Ihdir,

  Nice project! Here are a few suggestions:

  Another fun way to center a <div> is by using position: relative on the parent and position: absolute on the content, with top: 50%, left: 50%, and transform: translate(-50%, -50%). For a challenge, you could also center it with Grid, but Flexbox is definitely the easiest method in this case.

  I’d suggest improving your semantic structure a bit. For example, you’ve nested your component in many <div>s, when one main container for the card, combined with Flexbox on the body, would be sufficient. Using fewer <div>s makes your code easier to read and helps search engines better understand your page.

  I noticed you’re using both <h1> and <h2>. While this isn’t necessarily wrong, the <h1> tag should be reserved for the page title and should only appear once. For card components or other sections, using <h2> for headings and <p> for paragraphs would be more appropriate.

  Keep up the great work!

  Best, Teodor

  Marked as helpful

  1

  ---

  

  Mayank Maheshwari•190

  @IhdirPosted 2 months ago

  Hi @TedJenkler ,

  Thank you so much for the feedback and suggestions!

  I've seen people centering elements using top: 50%, left: 50%, and transform: translate(-50%, -50%), but I haven’t tried it out yet. I'll definitely give it a go and see how it works for this project. I'll also take your other advice into consideration, like improving the semantic structure and using proper heading tags.

  I really appreciate your input, and I'll work on applying these changes to improve my code!

  Best regards, Ihdir

  1
• 

  Huy Phan•2,300

  @huyphan2210Posted 2 months ago

  Hi, @Ihdir

  I reviewed your solution and have a few thoughts:

  • As Svitlana Suslenkova mentioned, you should apply min-height: 100vh to the body. This ensures that the body always covers the full height of the viewport, preventing it from being shorter than the screen and ensuring a consistent layout.
  • Your main tag contains two elements: one with the .Element class and one with the .attribution class. I'd recommend giving .Element a more descriptive name. Good naming conventions improve code clarity, especially in larger projects. You can explore CSS naming conventions like BEM (Block, Element, Modifier) for better structure.
  • You don’t need to use Flexbox for the card (.Element class, which is a block-level div). The child elements inside, except for the img, are also block-level, so they naturally stack on their own lines. Additionally, I’m not sure why you set width: 90% on the .Element class. On smaller viewports (under 37.5rem), it causes horizontal overflow. Simply removing this should resolve the issue.
  • Lastly, the styles max-height: 43.75rem, height: auto, and overflow: auto on the card seem unnecessary as they don’t contribute much to the layout.

  There are a few other areas for improvement, but I’ll keep this brief for now. 😉

  It looks like you’re just starting out in front-end development—good job so far!

  Hope this helps!

  Marked as helpful

  1

  ---

  

  Mayank Maheshwari•190

  @IhdirPosted 2 months ago

  @huyphan2210 "Thank you, Sir. I appreciate your feedback and will definitely work on the points you mentioned. I'm a 17-year-old passionate beginner, aiming to build a career as a full-stack developer. I started learning HTML and basic CSS just 11 days ago, and I’m excited to keep improving!"

  1
• 

  Svitlana Suslenkova•5,360

  @SvitlanaSuslenkovaPosted 2 months ago

  I see you added flex, but your project didn't align to the center. The problem is you should add to your flex also min-height: 100vh; Currently, the height of the body is the same as the component in it.

  Hope you found this comment helpful :)

  Marked as helpful

  1

  ---

  

  Mayank Maheshwari•190

  @IhdirPosted 2 months ago

  @SvitlanaSuslenkova “Thank you, Ma'am. I appreciate the feedback and will definitely look into it. I’ll work on addressing these points to improve my future projects too.”

  0