@Islandstone89
Posted
Hi Chris, good job!
I've inspected your code, and these are my suggestions to improve your solution even further - I hope you find them helpful :)
HTML:
-
Remove
role="main"
- the<main>
element has that role by default, so there is no need to declare it explicitly. -
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. -
Never have text in a
<div>
alone. "Scan the QR code" is a paragraph, so wrap it in a<p>
. I would also remove the<div>
, as it is not needed - divs are usually meant to group several elements for styling purposes. -
.attribution
should be a<footer>
, and you should use<p>
for the text inside. Move the<footer>
outside of the<main>
, as they are distinct landmarks.
CSS:
-
Including a CSS Reset at the top is good practice.
-
I recommend adding a bit of
padding
, for example16px
, on thebody
, to ensure the card doesn't touch the edges on small screens. -
On the
body
, changeheight
tomin-height: 100svh
- this way, the content will not get cut off if it grows beneath the viewport. Remove themargin
, it's already set using the universal selector (*
). Addflex-direction: column
andgap: 2rem
- the default value forflex-direction
isrow
, which makes the footer appear besides the main. Thegap
creates breathing room between the main and the footer. -
Remove the
width
andheight
inpx
on the card. We rarely want to give a component a fixed size, as we want it to grow and shrink according to the screen size. -
We do want to limit the width of the card, so it doesn't get too wide on larger screens. To solve this issue, the card needs a
max-width
, which should not be in%
but in rem. Around20rem
works well. -
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. -
Paragraphs have a default value of
font-weight: 400
, so there is no need to declare it. -
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 setting
max-width: 100%
on the image! It's also common to applydisplay: block
. -
Remove
position: absolute
, it is not needed.