Submitted 9 months ago

QR Code Component Challenge

Gab•170

@gab-holik

Preview site View code

This is a solution for...QR code component

HTML
CSS

1newbie

Design comparison

SolutionDesign

Solution retrospective

Thank you for any feedback.

Community feedback

solvman•1,650

@solvmanPosted 9 months ago

Very well done! 🎊🎉🚀

I have a few suggestions for you:

⭐️ Great job using semantic element <main>. Semantic elements significantly improve the SEO and accessibility of your project. First, the <main> landmark element represents the primary content of the document and expands on the central topic of the document. You should wrap your content in <main>. Such widgets as cards are more suited to be constructed with the <article> element, which encapsulates reusable, self-contained content. However, the <seciton> element is unnecessary since it has no semantic significance.
⭐️ Titles and headings are usually denoted by <h1>, <h2>, <h3>, and so on. Do not skip levels of headings. Regular text is generally encapsulated by <p>. A card-like widget's most appropriate heading level is likely <h2>.

With that being said, I would redo your code as so:

<body>
  <main id="container">
    <h1 class="visually-hidden">Frontend Mentor project submission</h1>
    <article class="card">
      <img src="./images/image-qr-code.png" alt="QR Code">
      <h2>Improve your front-end skills by building projects</h3>
      <p>Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p>
    </article>
  </main>
  <footer class="attribution">
      ... attribution goes here
  </footer>
</body>

As mentioned above, the <h2> heading is the most appropriate for the card-like widget. To avoid breaking hierarchy heading rules, I added an invisible <h1> heading to announce "Frontend mentor project submission" to accessibility users. Visually hidden class (it is also called sr-only which is "screen reader only") for the <h1> :

.visually-hidden {
  position: absolute;
  width: 1px;
  height: 1px;
  padding: 0;
  margin: -1px;
  overflow: hidden;
  clip: rect(0, 0, 0, 0);
  white-space: nowrap;
  border: 0;
}

Learn more about semantic HTML elements here

Please remember that block-level elements stack one on top of the other. The only element that is not block level within the card is <img>, which could be "converted" to block level through a simple reset, which should be used almost on every project anyways:

img {
    display: block;
    max-width: 100%;   /* ensures images does not overflow the container */
}

⭐️ The reset above will ensure the images stretch to fit the containing element. There is no need to set the image size; doing so makes the image not responsive to viewport resizing.
⭐️ Great job using Grid to place your card in the middle of the screen. 👍
⭐️ Be consistent using REM units for margin, padding, and font size. 👍
⭐️ Great use of custom global variables. 👍

Otherwise, very well done!🎊 Keep it up!👏 I hope you find my comments useful 🫶

Marked as helpful

Gab•170

@gab-holikPosted 9 months ago

@solvman Thank you so much for the feedback and for the tips I really appreciate it.

Craig•190
@cchiversPosted 9 months ago
Looks really good, I would drop the font-weight of the heading. Other than that it looks amazing. Keep up the great work!
Marked as helpful
1
Gab•170
@gab-holikPosted 9 months ago
@cchivers thanks for the feedback. You are right, for some reason I thought the heading was smaller than it should be.
0

Please log in to post a comment

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord