Design comparison
Solution retrospective
As a first challenge, I'm pretty proud of what I did in the CSS file, but I didn't spend enough time on the HTML file and forgot some basic steps.
What challenges did you encounter, and how did you overcome them?I struggled a little bit with the responsive part of the problem, but after I submitted a first solution, I received a lot of help to correctly finish the challenge.
What specific areas of your project would you like help with?I could use some practice to correctly understand all the Flexbox properties.
Community feedback
- @danielmrz-devPosted 9 months ago
Hello @Nomylim!
Your solution looks great!
I have a couple of suggestions (about semantic HTML) for improvement:
📌 First: Use
<main>
to wrap the main content instead of<div>
.Think of
<div>
and<span>
in HTML like plain boxes or placeholders. They're handy for holding content, but they don't tell us anything about what's inside or what it's meant for on the webpage.📌 Second: Use
<h1>
to<h6>
for titles instead of<p>
.The tag
<p>
is meant for paragraphs. For titles, we have the HTML headings (the tags<h1>
to<h6>
).Here's a quick guide on how to use them:
Unlike what most people think, it's not just about the size and weight of the text.
- The
<h1>
to<h6>
tags are used to define HTML headings. <h1>
defines the most important heading.<h6>
defines the least important heading.- Only use one
<h1>
per page - this should represent the main heading/title for the whole page. And don't skip heading levels - start with<h1>
, then use<h2>
, and so on.
All these tag changes may have little or any visual impact but they make your HTML code more semantic and improve SEO optimization as well as the accessibility of your project.
I hope it helps!
Other than that, great job!
Marked as helpful1 - The
- @Islandstone89Posted 9 months ago
Hello, Nomi. Well done completing your first project!
Here are some suggestions that hopefully will help you :)
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 the "main" section of a page. Change the first<div>
to a<main>
. -
It is common to use classes instead of IDs.
-
The alt text must also say where it leads(frontendmentor.io).
-
"Improve your front-end skills by building projects" is a heading. Change it to a
<h1>
. -
.attribution
should be a<footer>
, and its text must be wrapped in a<p>
.
CSS:
-
It's good practice to include a CSS Reset at the top.
-
Do not use
%
forborder
,margin
orpadding
, and usually not for widths either. -
Remove
border
andmargin
on thebody
. Changepadding
to1rem
, so the card doesn't touch the edges on small screens. -
Remember to declare a fallback font, in case the user can't access the specified font:
font-family: "Outfit", sans-serif;
. -
On the
body
, addmin-height: 100svh;
. This will make thebody
take up atleast the whole height of the viewport, while still being allowed to grow (setting aheight
would cause overflow if the content were to grow beneath the height). Thebody
is by default only as tall as its content. This might be a bit confusing, since thebackground-color
we give it fills the whole viewport. But if you were to givebody
a border, you would see that it doesn't grow taller than the content inside it. It does fill the entire width of the viewport by default, so there's an important difference to take note of. -
Also on
body
, addgap: 2rem;
to create some space between the<main>
and the<footer>
. -
Remove the
border
on the card. -
padding
on the card should be equal on all 4 sides. I would also increase it to around10px
. -
Remove the width and height on the card. Whether it's in
%
orpx
, setting fixed dimensions is generally a no-no. The problem with setting fixed sizes is that the web is not a fixed medium. Components must adapt to all kinds of screens, and they cannot do that with a fixed size. If the viewport happens to be smaller than the fixed size, it will cause overflow, meaning you would have to scroll horizontally to see all of the content. This is bad user experience, and it also looks terrible. -
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. -
Paragraphs have a default value of
font-weight: 400
, so there is no need to declare it. -
Remove everything on the image, except for
border-radius
. Adddisplay: block
andmax-width: 100%
- the max-width prevents it from overflowing its container. -
As the design doesn't change, there is no need for any media queries. When you do need them, they should be in rem, not px. Also, it is common practice to do mobile styles first and use media queries for larger screens.
Marked as helpful1@NomylimPosted 9 months agoHi @Islandstone89
Thank you so much for your feedback; it's really helpful. I'm currently making changes to my code and I happen to have a question. When you say that 'The alt text must also indicate where it leads (frontendmentor.io),' are you referring to the image properties or something else?
1@Islandstone89Posted 9 months ago@Nomy you simply have to add a few more words in the alt attribute on the image:
<img src="images/image-qr-code.png" alt="qr code that leads to the Frontend Mentor website." />
Marked as helpful0 -
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