Design comparison
Community feedback
- @Islandstone89Posted 10 months ago
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 container div into a<main>
, and give it a class instead of an ID. -
I would remove the rest of the divs, as they are not needed.
-
The alt text also needs to say where it leads (frontendmentor.io).
-
.attribution
should be a<footer>
. -
The footer text and the"Scan your" need to be wrapped in a
<p>
.
CSS:
-
It's good practice to include a CSS Reset at the top.
-
On
body
, changeheight
tomin-height
- this way, the content will not get cut off if it grows beneath the viewport. Addflex-direction-column
, and agap
of a couple of rem, to create some space between the<main>
and the<footer>
. -
Remove all widths and heights, both in
px
and%
. Except for icons, you rarely want to set fixed dimensions(especially heights), as this easily creates issues with responsiveness. -
You don't need
margin: auto
on the card since it is centered using Flexbox. -
Remove
margin
from the image - to create the space between the image and the edge of the card, setpadding
on all 4 sides of the card. -
Add
display: block
andmax-width: 100%
on the image - the max-width prevents it from overflowing its container. -
Remove
positioning: relative
and its corresponding properties.
Marked as helpful0@Ahmed-Mustafa132Posted 10 months agoHi @Islandstone89 Thank you for your feedback I modified my solution and You can take a look to see if this solution is correct or not
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