Design comparison
Community feedback
- @Islandstone89Posted 4 days ago
Hi, well done!
Here are some tips:
HTML:
-
It's perfectly OK to use a
<div>
as the card. A<section>
is not semantic unless you add aaria-labelledby
where the value is theid
of the section heading. -
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.
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. -
It's not common to set anything on
html
. Movefont-family
to thebody
, and removefont-size: 10px;
- even though changing it makes it easier to convert frompx
torem
, it's not recommended. -
.container
doesn't need any styles either, so move the properties tobody
. Removewidth: 100%
- block elements likebody
,div
,main
etc take up the full width of their parent by default. Changeheight
tomin-height: 100svh
- this way, the content will not get cut off if it grows beneath the viewport. -
To center the card horizontally and vertically, you can also use Flexbox on the body:
display: flex; flex-direction: column; justify-content: center; align-items: center; min-height: 100svh;
-
Now that you've removed
font-size: 10px
on thehtml
, you must adjust themax-width
on the card to20rem
, which equals320px
. -
Paragraphs have a default value of
font-weight: 400
, so there is no need to declare it. -
On the image, add
height: auto
and changewidth
tomax-width: 100%
- the max-width prevents it from overflowing its container. Without this, an image would overflow if its intrinsic size is wider than the container.max-width: 100%
makes the image shrink to fit inside its container. -
To create the space between the image and the edge of the card, set
padding
on all 4 sides of the card:padding: 16px;
.
Marked as helpful1@Codeur-OmniscientPosted 1 day ago@Islandstone89 Thank you for the feed-back i learn a lot I’ll make a modification soon. I don’t know too much about semantic html if you have some resources shared me that it would be helpful 😉
1@Islandstone89Posted 1 day ago@Codeur-Omniscient Here's an article about semantic HTML from the Google Chrome team.
0 -
- @v-t-9Posted 4 days ago
Hello!
Congrats on completing the challenge! I noticed something you might find interesting. Consider using semantic HTML tags like <section></section> instead of <div> to wrap the QR card. Some of the benefits of this are: improved search engine rankings and better maintainability.
Hope you find this helpful!
1 - @TheSecondChancePosted 5 days ago
HTML elements effectively, which enhances both accessibility and SEO. Elements like <header>, <main>, <section> Responsive Layout: The layout looks great across various screen sizes, adapting seamlessly from mobile to desktop.
1
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