Perfumes Preview Card

Community feedback

• 

  Sounak Mukherjee•560

  @kanuosPosted almost 2 years 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

  1. Your code is missing the landmark wrapper. Easy fix - convert the div.container to main.container
  2. 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.
  3. Always use an alt attribute to your img element.
  4. 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 on img elements. Setting the display to block helps prevent a lot of unnecessary bugs.
  5. 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.
  6. 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.
  7. 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.
  8. Try using relative units like rem, %, em etc instead of absolute units like px.

  Hope this helps. Hope to see a lot more from you in the future. Happy coding :)

  Marked as helpful

  1

  ---

  

  Kamil Wiater•230

  @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
• 

  Anna P. M.•390

  @annapmarinPosted almost 2 years 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 helpful

  1
• 

  D. Jaime Blockton•180

  @digigrrl525Posted almost 2 years 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