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.container
into a<main>
. -
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).
-
"Improve your" is a heading, not a paragraph. "Scan the QR code" on the other hand, is a paragraph, and must be a
<p>
, not a<span>
. -
.attribution
should be a<footer>
, and its text must be wrapped in a<p>
.
CSS:
-
You don't need to put anything on
html
, only thebody
. I would move all the properties on.container
over on thebody
, and changeheight
tomin-height
- this way, the content will not get cut off if it grows beneath the viewport. Also, addflex-direction: column
andgap: 2rem
. -
font-size
must never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. -
The card should not have a
width
, only amax-width
of around20rem
, to prevent it from getting too wide on larger screens. -
On the image, add
display: block
and changewidth
tomax-width: 100%
- the max-width prevents it from overflowing its container. -
Paragraphs has a default value of
font-weight: 400
, so there is no need to declare it. -
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@abdelrahmenPosted 10 months ago@Islandstone89 Wow, thank you so much for your precious observations, i will update the solution as per your points and surely use them in my future code.
1@abdelrahmenPosted 10 months ago@Islandstone89 hey, I did all of your points, plus, I changed
height: 100%
in thebody
tomin-height:100vh
, when it was 100% it didn't take the full height and i do not understand why, if you know please tell me, also i have another question, when the width of the screen falls under 150 px, the card overflows to the left, how can I fix that?0@Islandstone89Posted 10 months ago@abdelrahmen no screens are less than 150px wide, so that's not a problem.
0 -
- @sperrowPosted 10 months ago
Looks great on desktop. To design for a mobile layout, you can use Chrome/Safari/Firefox's device emulator to display your page how it would look on different screen sizes.
1@abdelrahmenPosted 10 months ago@sperrow thank you, I didn't test it on a Real mobile phone, I just toke the dimensions specified in the design guide and went for it.
1@sperrowPosted 10 months ago@abdelrahmen makes sense, just fyi if you know how to use Chrome's dev tools/inspector it's really easy to switch the screen size so you can see what it'd look like without having to use an actual phone: https://developer.chrome.com/docs/devtools/device-mode
0@abdelrahmenPosted 10 months ago@sperrow that's what i did to make it work on a width of 375px as specified in the design guideline, but surely it needs more editing to work on other screen sizes, as most phones nowadays are wider than this and won't trigger the current media query.
1@sperrowPosted 10 months ago@abdelrahmen ahh I see haha, sorry I misunderstood. Perhaps you could just change your media query to a wider breakpoint (e.g. 768 instead of 375) so that it will still look good at 375px but also good on wider phones/tablets.
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