Design comparison
Solution retrospective
Had problems with a proper positioning of the image (please see the bottom line).
Any feedback concerning the above (and other aspects) hugely appreciated :))
Community feedback
- @kanuosPosted over 1 year ago
Hi Kamil, congrats on your submission. Your code looks pretty decent to be honest but like they say - there's always room for improvement.
Here's a few tips on how to improve your code
- Your code is missing the landmark wrapper. Easy fix - convert the
div.container
tomain.container
- Looking at your class names, try to follow a standard - eg. BEM. Look up
BEM naming convention
. That'll help you isolate the UI into minute re-usable components - hence leading to DRY-er code. - Always use an
alt
attribute to your img element. - Try to make the img styling more generic. The starter kit provided by FEM is kind enough to resize and optimize the images. What if the images were out of shape/aspect ratio? Use
object-fit
object-position
etc props onimg
elements. Setting the display toblock
helps prevent a lot of unnecessary bugs. - If you look at the design, you'll see the font weight of the heading element is not optimal in your code. Fix it when you can. While at it, fix the font size of the card description too.
- You probably missed out the hover effect on the button. As a pointer, I'd recommend using a
<a>
tag instead of the button element. Also, decrease the border-radius of the button to make it look identical to the design's button. - If you look at the desktop design, the image on the left and the card info on the right are of the same width. In your solution, the image looks a bit thinner compared to the card info. Also, the padding on the text content is a bit more in the design.
- Try using
relative
units likerem, %, em
etc instead of absolute units likepx
.
Hope this helps. Hope to see a lot more from you in the future. Happy coding :)
Marked as helpful1@WiaterKaPosted over 1 year ago@kanuos your feedback is impressing! I partially managed to introduce those changes to the project. I will bear in mind your helpful tipis for the future projects :))
I really appreciate your time and effort - it helped a lot! Hope to see you around here in the future.
Have a nice day and happy coding!
1 - Your code is missing the landmark wrapper. Easy fix - convert the
- @annapmarinPosted over 1 year ago
Good job! Maybe you can erase the
text-align: justify
to fit the original design. Also, regarding the image, you can heighten the image:height: 455px;
to fit in better, or you can try adding new properties to the div you named as "img_container".Also (but this is less important) you can add more right margin to the <h1> and more line-height and margin-right to the product description <p> to fit more the original design.
Marked as helpful1 - @digigrrl525Posted over 1 year ago
It's looking good. I don't know that I would have used an SVG for the button, but that is a personal choice. Also, you have "Add do Cart" instead of "Add to Cart" as your button CTA.
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