Very well done! 🎊🎉🚀
I have a few suggestions for you:
-
⭐️ Great job using
<main>semantic element! 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. The attribution piece most likely fits to be encapsulated by the<footer>semantic element. -
⭐️ 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
You don't need to use Flex for the card contents. Please remember that block-level elements stack one on top of the other. The only element not block level within the card is <img>, which could be "converted" to block level through a simple reset, which you already did. I suggest you not hard-set the image size since it makes it not responsive to viewport size changes.
-
⭐️ You don't need
margin-left: autoandmargin-right-auto. You can center the text by doingtext-align: center; -
⭐️ Great job using Flex to place the card in the middle of the screen. It is a perfectly viable choice. There is another method that does the same. I prefer the Grid since it is only a line shorter:
body {
height: max(100vh, 700px);
display: grid;
place-content: center;
}
-
⭐️ Consider 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
@upovibe
Posted
@solvman thanks for the clearification
