Product preview card component

Community feedback

• 

  Grego•1,310

  @Grego14Posted 7 months ago

  Hello! 🎉 Congratulations on completing the challenge! 🎉

  I've been reading your code and here are some recommendations:

  I think you made a mistake in the body and added min-width: 100vh instead of min-height: 100vh.

  After doing what I said above it is no longer necessary to add a margin: 12em auto to the body using media querys.

  You can use the margin-inline property to specify the margin of the sides and avoid doing this kind of thing:

  .container{
    margin-left: 10em;
    margin-right: 10em;
  }
  

  Or you can also do something like this:

    margin: 0 10em; 
  

  Where the first value would be top and bottom and the second value would be left and right.

  It is not necessary to add some values ​​again to the .container when you use media queries such as background-color, display, flex-direction... since they are already being added without the use of media querys.

  I recommend you set a max-width to the .container element: max-width: 700px.

  You are using two img elements to switch between the mobile image and the desktop image, I recommend that you read about this html element picture will help you manage those images in a better way!

  You can add a max-width: 50% to the image so that it takes up half the width of its parent.

  You are using a h4 in the text PERFUME, I recommend that you do not skip headings, for that text you can use a span or p element.

  You should not use the b or s element for styling, it is better to use CSS properties like font-weight: bold or text-decoration: line-through in another element like p or span.

  I hope all these recommendations are helpful to you! 😁

  Marked as helpful

  0

  ---

  

  Mihai Niculae•250

  @mihainrsPosted 7 months ago

  Heyyy @Grego14!

  Thank you for pointing it all out for me. I definitely see it now. I've been a bit frustrated with the switch from the mobile version to the desktop one due to some padding issues I was having while working on the CSS part, and in my attempt at fixing it up, I've left that unfixed.

  The picture element definitely seems like a better alternative haha. I just recently read that it's possible to swap pictures out like that (just never thought of having mutliple pics and hiding them as needed throughout the project)

  Stylizing elements was my alternative to not use too many classes and id's, since I wouldn't wanna get overwhelmed by my own code. Trying to see if things work out, but knowing that now will definitely help me build better practices.

  Also, just realised that the solution preview on frontend mentor looks different than the one on the project's website for some reason- will have to look into that, unless it's a bug that I'm not aware of.

  Thank you for taking the time to let me know about it all!!

  1