Design comparison
Community feedback
- @skipperr254Posted 4 months ago
Hi there Khushi, good job on the project.
-
I noticed you used the with property in the body element and set it to 100%. In my experience and advice from experts, it's usually not necessary and everything works if it's omitted completely. It also adds the scroll effect even on a page that it is not necessary at all.
-
Also, didn't the
justify-content: center;
work or why did you comment it out? I think it would have centered the card element. If you were being adventurous then its okay. Just wanted to know. -
I missed the background color/colour of the body too, lol! Not sure where I went wrong but will look at it later.
-
Also, when I submitted my solution, I was pushed to correct on accessibility by using HTML5 landmarks like header, main, footer, etc.
1 -
- @Islandstone89Posted 4 months ago
Hi! Good job.
Here are some things that need to be improved. Please don't be disheartened - we all start somewhere :)
HTML:
-
Every webpage needs a
<main>
that wraps all of the content, except for<header>
andfooter>
. This is vital for accessibility, as it helps screen readers identify a page's "main" section. Wrap the card in a<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."
-
Headings should always be in order, so you never start with a
<h3>
.As the card heading would likely not be the main heading on a page, I would change it to a<h2>
. -
.attribution
should be a<footer>
, and you should use<p>
for the text inside.
CSS:
-
It is best practice to write CSS in a separate file, often called
style.css
. Create one in the same folder as theindex.html
, and link to it in the<head>
:<link rel="stylesheet" href="style.css">
. -
Including a CSS Reset at the top of the stylesheet is good practice.
-
Use the style guide to find the correct
background-color
. -
I like to add
1rem
ofpadding
on thebody
, to ensure the card doesn't touch the edges on small screens. -
You don't need
width: 100%
on thebody
- it is a block element, meaning it takes up the full width by default. -
On the
body
, changeheight
tomin-height: 100svh
- this way, the content will not get cut off if it grows beneath the viewport. -
On
body
, uncommentjustify-content: center
. -
On
body
, addgap: 2rem
, to create some space between the main and the footer. -
Remove the margin on the card.
-
Remove all widths and heights. Fixed sizes set in
px
are not recommended. -
Add a
max-width
of around20rem
on the card, to prevent it from getting too wide on larger screens. -
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. -
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. -
On the image, add
display: block
andmax-width: 100%
- the max-width prevents it from overflowing its container. -
I would make the corners of the image the same as the card:
border-radius: 20px
. -
Remove
position: fixed
on the footer.
0 -
Please log in to post a comment
Log in with GitHubJoin 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