Order-Summary Component built with HTML / CSS :)

Community feedback

• 

  hitmorecode•6,680

  @hitmorecodePosted over 1 year ago

  Nice well done. Just a few remarks and things you can fix.

  • Change min-height: 90vh to min-height: 100vh to use the entire height of the page.
  • You have on .order__container a margin-top of 1rem. You can remove this, it's not necessary.
  • When naming classes and id's, try to stick to one naming convention (this is my personal preference). When using different naming conventions it's easier to make a mistake. Right now you are mixing classe names with "--" and with "__"
  • You should lower the width of the card for small screen sizes. Right now the width stays the same for large and small screen sizes. This is a problem if the screen size goes below 450px.
  • Add to the media query card width is equal or lower than 375px for small screens

  @media screen and (max-width:768px){
  /* here you are telling the browser to hide the background image, but on the next rule you are telling the browser to display an image. You can remove all these rules in the media query */
   body{
      background-image: none; 
      width:100%; /* you don't need this line. the width of the body is always 100% */
    }
  
    .mobile--bg--img{
      display:block;
      background-size: cover;
      background-repeat: no-repeat;
    }
  
    .order--btn{
      padding:0.9em 5em;
    }
  }
  

  A much easier and better way to show and hide picture is to use the picture element.

  <picture>
      <source media="(min-width: xxxx)" srcset="location of the image for desktop">
      <source media="(min-width: xxxx)" srcset="location of the image for mobile">
      <img src="location of the image for desktop">
  </picture>
  

  With this the browser will show and hide the image according with the given screen size.

  0

  ---

  

  Favour Ukonu•60

  @Thatgirl9Posted over 1 year ago

  @hitmorecode Thank you very much, I'll work on that 🙌

  0
• 

  kudos2Shef•170

  @kudos2ShefPosted over 1 year ago

  Hi @Thatgirl9,

  Congratulations on completing this challenge! Your solution is perfect. This card component project of yours is responsive itself, so you need not worry about it.

  There is only one thing in your media query that I would recommend you to change. In your media query, the mobile background image is not appearing. So, in the body segment, instead of using background-image: none; try using the following code:

  body {
      background-image: url(./order-summary-component-main/images/pattern-background-mobile.svg);
      width: 100%;
  }
  

  Also, please remove the background-image element from the HTML as it is not needed:

  <background-image src="./order-summary-component-main/images/pattern-background-mobile.svg" class="mobile--bg--img" alt="mobile-background-img"></background-image>
  

  Additionally, it seems that you forgot to add the Frontend Mentor icon in your code. You can include it using the following link tag:

  <link rel="icon" type="image/icon" href="images\favicon-32x32.png">
  

  I hope this helps! Great job on the project, and keep up the excellent work!

  0

  ---

  

  Favour Ukonu•60

  @Thatgirl9Posted over 1 year ago

  @kudos2Shef Thank you, I'll work on that. Thank you for the feedback!🙌

  0