Product Preview Card

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted about 2 years ago

  Hi @naomichoe 👋, good job completing this challenge, and welcome to the Frontend Mentor Community! 🎉

  I like this solution for the challenge. Here are a few suggestions I've made that you can consider in the future if you're looking to improve the solution further:

  • You can use a <picture> tag when you need to change an image in different viewports. Using this tag will prevent the browser from loading both images, saving bandwidth and preventing you from utilizing a media query to modify the image.

  <picture>
     <source media="(max-width: 600px)" srcset="./images/image-product-mobile.jpg">
     <img src="./images/image-product-desktop.jpg" alt="your_alt_text">
  </picture>
  

  • Try to use semantic tags in your code. It should have the main tag that groups all of the product card elements.
  • The cart icon is for decoration purposes only, so it can be hidden from screen-readers by adding aria-hidden="true" and leaving its alt attribute empty:

  <img src="./images/icon-cart.svg" alt aria-hidden="true">
  

  • The container isn't centered correctly. You can use flexbox or grid layout to center elements:

  Using grid layout:

  body {
     width: 100%;
     min-height: 100vh;
     display: grid;
     place-content: center;
  }
  

  Links with more information:

  • The Complete Guide to Centering in CSS.
  • A Complete Guide to Flexbox (CSS-Tricks).
  • How TO - Center Elements Vertically (W3Schools).
  • CSS Layout - Horizontal & Vertical Align (W3Schools).

  .

  • With flexbox, you can provide a maximum width for the main-container, and add the property flex: 50%; to .product-img and .info-container, so you are creating two columns. More information.

  .main-container {
      max-width: 600px; /* Sets a maximum width */
      display: flex;
      justify-content: center;
      margin: 100px auto;
  }
  
  .product-img, .info-container {
      flex: 50%;
  }
  
  .product-img {
      /* max-width: 300px; */
      /* max-height: 450px; */
      /* flex-shrink: 0; */
  }
  
  .info-container {
      flex: 50%;
      background-color: white;
      /* max-width: 300px; */
      border-radius: 0 10px 10px 0;
      padding: 30px;
      /* flex-shrink: 0; */
  }
  

  I hope those tips will help you.

  Good job, and happy coding!

  Marked as helpful

  1

  ---

  

  naomichoe•60

  @naomichoePosted about 2 years ago

  @MelvinAguilar Thank you! Your tips are already very helpful! I originally tried using the <picture> tag with srcset & sizes but was not getting the correct mobile output but your tip looks more helpful so I am going to try to redo it with the <picture> tag. And I will also need to familiarize myself with the grid layout more, I've been focusing more on flexbox. Thank you again!

  0
• 

  Christopher Alvarez•430

  @ChrisesbuenoPosted about 2 years ago

  Hello, good code, as a piece of advice I would tell you to add the following styles to the button, cursor: pointer; font-family: inherit; This is because the correct font is not being applied to the button and changing the cursor to a pointer is a good practice to indicate that it is a button and can be clicked. In the product-info styles I would also add a line-height to make it look more spaced, everything else is quite correct. very good code :D

  Marked as helpful

  1

  ---

  

  naomichoe•60

  @naomichoePosted about 2 years ago

  @Chrisesbueno Thank you!! I should have checked for that font and cursor in the button! Now that I noticed, do you know how I can get my svg icon to be more centered versus aligned with the bottom of the text like it is now?

  0
• 

  Adriano•38,080

  @AdrianoEscarabotePosted about 2 years ago

  Hi naomichoe, how are you?

  Welcome to the front-end mentor community!

  I really liked the result of your project, but I have some tips that I think you will enjoy:

  • every Html document must contain the main tag, so we can identify the main content, to fix this, wrap all the content with the main tag. HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
  • To align some content in the center of the screen, always prefer to use display: flex; it will make the layout more responsive!

  Example:

  body {
      margin: 0;
      padding: 0;
      display: flex;
      align-items: center;
      justify-content: center;
      min-height: 100vh;
  }
  

  The rest is great!

  I hope it helps... 👍

  Marked as helpful

  0