@Islandstone89
Posted
Hi, here is some feedback.
HTML:
-
Good job on including the
<main>, most beginners are not aware of its importance. However, you have way too many divs. In such a simple component, you only need a<main>, which can also act as the card. So, I would remove all divs. -
The image must have alt text. This is essential for screen readers to understand the image. The alt text should be descriptive, and in this example, it also needs to say where it leads (frontendmentor.io).
-
"Scan the QR code" and the footer text should both be wrapped in a
<p>. -
Hence, you should end up with the following HTML structure:
| <body>
|| <main>
||| <img>
<h1>
<p>
|| </main>
|| < footer>
||| <p>
|| </footer>
CSS:
-
Performance-wise, it's better to link fonts in the
<head>of the HTML then using@import. -
It's good practice to include a CSS Reset at the top.
-
Remove
width: 100%onhtml,mainandbody- they are 100% wide by default, because they are block elements. -
Change
height: 100vhtomin-height: 100vh. And you only need to put it on thebody. -
Background color and text alignment should be moved from the card to the
body. Additionally, add Flexbox to center the card horizontally and vertically:
background-color: hsl(212, 45%, 89%);
text-align: center:
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100vh;
-
max-widthon the card is way too big, and should not be in px but in rem. 1440px equals to 90 rem, but around 20rem should be all you need. -
Remove
width: min-content. -
Font-size must never be in px. Use rem instead.
-
Remove the fixed width and height on the image. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.
-
Do add
display: blockandmax-width: 100%on the image. The max-width prevents the image from overflowing the card.
Marked as helpful
@amakura-411
Posted
@Islandstone89
Thank you for the clear feedback, I appreciate it!
I've improved the code based on your advice.
However, there's one thing I'm not clear about.
You suggested using max-width in rem instead of px. You mentioned that 1440px equals 90rem, but recommended using 20rem. How did you determine max-width: 20rem?
I want to know how you arrived at that. (It might be adjustments based on actual screen view, but...)
I'd appreciate it if you could provide an answer.
Thank you in advance for your feedback!
@Islandstone89
Posted
@amakura-411 Happy to help :)
If you open DevTools (click F12 in Chrome), you can select "Responsive" in the list of simulated devices. Then, you can adjust the screen size smoothly, and see where the card starts to get too big.
Marked as helpful
@amakura-411
Posted
@Islandstone89
Thank you for your response!
Thanks to you, it looks like I'll be able to write some good code!!
I'm grateful to have met someone as kind as you! :))
