Responsive qr code component using CSS grid

Please log in to post a comment
Log in with GitHubCommunity feedback
- P@Islandstone89
Hey, great job!
A few suggestions for improvement:
HTML:
-
<main>
holds all of the main content on a page. As a card would likely not be the only component on a page, I would wrap the card content in a<div class="card">
inside of<main>
. -
The alt text must also say where it leads(the frontendmentor website). A good alt text would be "QR code leading to the Frontend Mentor website."
-
I would change the heading to a
<h2>
- a page should only have one<h1>
, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components.
CSS:
-
I recommend adding a bit of
padding
, for example16px
, on thebody
, to ensure the card doesn't touch the edges on small screens. -
Remove the margin on the card.
-
Remove
width: 100%
andheight: 100%
onhtml, body
. It's not common to set any styles onhtml
, and thebody
is 100% wide by default, as it is a block element. We do need thebody
to take up the full height, as by default it is only as tall as its content. We could addheight: 100svh
, but that will cause overflow if the card grows taller than the viewport. So, instead we need to setmin-height: 100svh
. -
You could also center the card using Flexbox:
display: flex; flex-direction: column; justify-content: center; align-items: center;
-
Since all of the text should be centered, you only need to set
text-align: center
on the body, and remove it elsewhere. The children will inherit the value. -
It's also common to set
height: auto
on images. -
As the design doesn't change, there is no need for any media queries. When you do need them, they should be in
rem
orem
, notpx
.
Marked as helpful -
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