@kanuos
Posted
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 helpful
@WiaterKa
Posted
@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!