@Islandstone89
Posted
Hi, you have done a decent job. Here is my feedback.
HTML:
-
You need a
<main>, this is important for accessibility. Since the card is the only "main" content on the page, the<main>could also be the card itself. I would change.containerinto a<main>, and removesection>, you don't need that element here. -
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).
-
I see you have commented out the
.attribution. If you were to include it, it should not be a<div>, but a<footer>, with its text being wrapped in a<p>. But it's OK to omit it, too.
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.
-
Do not set
font-size: 62.5%on thehtml>. -
Since all text is center-aligned, you can set
text-align: centeron the body, and remove it elsewhere. -
You are mostly using rem for font sizes, that's good. If you include it,
font-sizeon.attributionshould also be in rem. -
To center the card horizontally and vertically, use Flexbox on the
body:
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100vh;
- Hence, you don't need
automargin on.container- but change 5rem to 1rem, to prevent the card from touching the edge of the screen on smaller sizes.
Marked as helpful
@Alex-coding-3420
Posted
@Islandstone89 Thank you for the feedback, and thanks for linking the article regarding setting the font-size to 62.5% on the HTML. I'll take all this feedback for my next challenge attempt.
