Design comparison
Solution retrospective
Leave me your opinion about my solution to this challenge. Do you think I could have done it differently? What would you change to my code?
Community feedback
- @ecemgoPosted over 1 year ago
Some recommendations regarding your code that could be of interest to you.
- If you want to make the card centered both horizontally and vertically, you'd better add flexbox and
min-height: 100vh
to thebody
body { background-color: hsl(212, 45%, 89%); /* width: 100vw; */ /* height: 100vh; */ display: flex; flex-direction: column; justify-content: center; align-items: center; min-height: 100vh; font-family: Outfit, sans-serif; }
- If you use
max-width
, the card will be responsive and you can reduce the width a bit - You'd better add
padding
to give a gap between the content and the border of the card
.container { /* width: 320px; */ max-width: 300px; /* height: 497px; */ padding: 16px; }
- In addition to that above, in order to make the card responsive and the image positioned completely on the card, you'd better add
width: 100%
to the img
.container img { /* display: flex; */ /* width: 288px; */ /* height: 288px; */ /* margin: 16px auto; */ width: 100%; border-radius: 10px; }
- If you define repetitive styles such as
text-align
,font-family
in the parent element, you won't define it again and again. Thus, your code will be clean. - You can update texts to avoid repetition in this way
.text-container h1 { /* font-family: Outfit, sans-serif; */ /* text-align: center; */ /* font-size: 23px; */ font-size: 18px; }
.text-container p { /* font-family: Outfit, sans-serif; */ /* text-align: center; */ /* margin: 22px; */ margin: 20px 0; font-size: 15px; color: hsl(220, 15%, 55%); }
- Finally, if you follow the steps above, the solution will be responsive. Additionally, you can remove
main
and media queries to write less code.
/* main { display: flex; align-items: center; justify-content: center; width: 100%; height: 95%; } */ /* @media (max-width: 400px) { body { width: 375px; height: 667px; } } */
Hope I am helpful. :)
Marked as helpful1@sdmatiasPosted over 1 year ago@ecemgo Thank you for your answer it is really very helpful. I tried to make my code as clean and simple as possible. With your recommendations it will be much better.
Mati
1 - If you want to make the card centered both horizontally and vertically, you'd better add flexbox and
- @Cyber-ChicPosted over 1 year ago
Your solution looks awesome! Wonderful job completing your first challenge! 😊
I have one small recommendation that may improve your site:
- Add a summary to your alt attribute. The QR code on your site will be scanned. This means that the image is not considered decorative and will require a description. To fix this you could change your code from alt=" " to alt="QR code to frontendmentor.io".
I hope this is helpful! Great work & Happy Coding!
-Angie
Marked as helpful1@sdmatiasPosted over 1 year ago@Cyber-Chic Thanks you for your answer, recommendation and good vibes in the comment!
Mati
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