Design comparison
Solution retrospective
Please provide your valuable feedback on my project. Specifically, I would like to know the following:
- How is the overall look and feel of the project?
- Is the content clear and easy to understand?
- Are there any suggestions you have for improving the card design or user experience?
- Are there any areas of the project where you experienced difficulty or confusion?
Community feedback
- @ecemgoPosted over 1 year ago
Some recommendations regarding your code that could be of interest to you.
If you want that this solution is responsive, I recommend some techniques without using media query for this solution but it's up to you whether you use it or not. Also, I recommend you try to avoid repetition in your code.
- You don't need to use
width: 100vw
in thebody
for this solution
body { /* width: 100vw; */ }
- When you use flexbox in the
body
for this solution, you don't need to use flexbox in the.container
to center the card - If you use
max-width
, the card will be responsive - You'd better update
padding
in this way and you can addmargin-bottom
to give gap between the card andBack to Index
.container { background-color: hsl(0, 0%, 100%); /* max-width: 85%; */ max-width: 300px; /* padding: 1rem 0; */ padding: 1rem; border-radius: 20px; /* display: flex; */ /* flex-direction: column; */ /* align-items: center; */ /* justify-content: center; */ margin-bottom: 1rem; }
- In addition to that above, in order to make the card responsive and the image positioned completely on the card, you'd better add
max-width: 100%
to the img
.container img { /* max-width: 92%; */ max-width: 100%; border-radius: 12px; }
- If you add width to the text, it prevents the card will be responsive
.container h1 { /* max-width: 90%; */ }
.container p { /* max-width: 88%; */ }
- If you follow the steps above, the solution will be responsive
- Finally, you don't need
.qrCard
and you can remove it to avoid repetition
/* .qrCard { width: 375px; height: 667px; background-color: hsl(212, 45%, 89%); display: flex; justify-content: center; align-items: center; } */
Hope I am helpful. :)
0 - You don't need to use
- @0xabdulkhaliqPosted over 1 year ago
Hello there 👋. Congratulations on successfully completing the challenge! 🎉
- I have other recommendations regarding your code that I believe will be of great interest to you.
BODY MEASUREMENTS 📐:
- The
width: 100vw
property forbody
element is not necessary. because it's a block level element which will take the full width of the page by default.
- Use
min-height: 100vh
forbody
instead ofheight: 100vh
. Setting theheight: 100vh
may result in the component being cut off on smaller screens.
- For example; if we set
height: 100vh
then thebody
will have100vh
height no matter what. Even if the content spans more than100vh
of viewport.
- But if we set
min-height: 100vh
then thebody
will start at100vh
, if the content pushes thebody
beyond100vh
it will continue growing. However if you have content that takes less than100vh
it will still take100vh
in space.
.
I hope you find this helpful 😄 Above all, the solution you submitted is great !
Happy coding!
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