@Islandstone89
Posted
Hi Danny, good job!
Here are some suggestions :)
HTML:
-
Remove the
<section>
, it is not needed. You only need the<div>
holding the card content inside the<main>
element. -
You should not include words like "image" or "photo" in the alt text. Screen readers start announcing images with "image", so an alt text of "image of qr code" would be read like this: "image, image of qr code". 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. -
Wrap the footer text in a
<p>
.
CSS:
-
Including a CSS Reset at the top is good practice.
-
"Outfit" is a sans-serif font, so change
--ff-primary: "Outfit", serif;
to--ff-primary: "Outfit", sans-serif;
. -
I recommend adding a bit of
padding
, for example16px
, on thebody
, to ensure the card doesn't touch the edges on small screens. -
Move
font-family
andfont-size
fromhtml
tobody
. -
On
body
, removefont-weight: 400
andoverflow: hidden
, as none are needed. -
Remove all styles on
.qr-code
as the<section>
is removed. -
Remove the margin on the card.
-
To center the card horizontally and vertically, with some space between the
<main>
and the<footer>
, I would use Flexbox on the body:
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100svh;
gap: 2rem;
-
Remove
width: 100%
on the card - a<div>
is a block element, meaning it takes up the full width by default. -
max-width
on the card should be in rem. Around20rem
will work fine. -
font-size
must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. -
letter-spacing
must also not be inpx
. You can useem
, where1em
equals the element's font size. Whenever the value is zero, you don't need to include the unit, so you would writeletter-spacing: 0
. -
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. -
Well done for setting
max-width: 100%
andheight
auto in images! It's also common to applydisplay: block
- its default value ofdisplay: inline
might cause unwanted space underneath an image if wrapped in a<div>
.
Marked as helpful
@Danny-Agyei
Posted
@Islandstone89 : Thanks for the feedback. I'll surely look into that and update the code.