2nd attempt-product-design

Community feedback

• 

  Anton•515

  @antaryaPosted about 2 years ago

  Hi ,

  Great job, it looks good.

  I have a couple of suggestions:

  CSS

  [1. All elements inside .whole have margin-left: 40px. To achieve the same but to have only one style, you can define padding on the .whole element. It will be easier in the future to change it instead of specifying it for each child element.

  /* remove all margin-left: 40px on all child elements and add one padding to parent */
  .whole {
    padding: 17px 40px;
  }
  

  [2. The content inside the Add to Cart button is not aligned vertically; it looks like the fixed height is redundant.

  .whole .fa {
    /* width: 50px; */
    /* height: 100px; */
    margin-right: 20px;
  }
  

  [3. You have fixed width and height which works in this case, but you cannot depend on text length; in real applications, you never know the size of the length, so you need to make your layout adapt to whatever length it will have. You can check it by typing longer text for an item description. Allow elements to grow without fixed width/height, and when you want to restrict, use max-width/max-height or min-width/min-height;

  [4. On smaller screens, e.g. 320px, the page doesn't look like in mock. You might need to use media queries, so it looks good on smaller screens.

  • https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries,
  • https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Responsive/Mobile_first

  And for arranging items, try using flexbox or grid as it is easier to use and has more features.

  • https://css-tricks.com/snippets/css/a-guide-to-flexbox/
  • https://www.frontendmentor.io/resources search for flexbox and grid for useful learning material.

  Using a mobile-first approach and media queries, you will change styles to:

  .container {
    /*
        A
        B
      */
    display: flex;
    flex-direction: column;
  
    /* you can set radius on parent element instead of each child element */
    border-radius: 10px;
    overflow: hidden;
  }
  @media (min-width: 900px) {
    .container {
      /*
          A B
        */
      /* display: flex; */
      flex-direction: row;
    }
  }
  

  • https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Responsive/Mobile_first

  [5. You can set display: block on the image to remove white space under the image. For details, check:

  • https://stackoverflow.com/questions/31444891/mystery-white-space-underneath-image-tag/31445364#31445364

  HTML

  [6. Wrap your card container with the main tag instead of using the main tag as your card parent tag; this would be semantically correct.

  • https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/
  • https://www.w3schools.com/html/html5_semantic_elements.asp

  Let me know if you have questions. I hope this will be helpful.

  Keep up the good work . Cheers!

  Marked as helpful

  0