Order Summary Card

Community feedback

• 

  👾 Ekaterine Mitagvaria 👾•7,860

  @catherineisonline

  Posted over 2 years ago

  Looks cool! I would add some transitions for hover states so the color is changing more smoothly. Also, to remove the report issues might need to wrap the div with class attribution in the footer instead of div

  10

  Marked as helpful
• 

  Adriano•42,890

  @AdrianoEscarabote

  Posted over 2 years ago

  Hello Ari Griggs, how are you? I truly loved your project's outcome, however I have some advice that I hope you'll find useful:

  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;
      flex-direction: column;
      justify-content: center;
      min-height: 100vh;
  }
  

  To improve the code structure wrap this div:

  <div class="attribution">

  with the semantic tag footer

  The remainder is excellent.

  I hope it's useful. 👍

  10

  Marked as helpful
• 

  Fernando Pérez•480

  @fermop

  Posted over 2 years ago

  Hi Ari, you did a good job! your page looks so good.

  As Hassia said, give the body a background-size of contain. To be honest, I can't understand the differences between contain and cover but remember that fortunately the documentation is in the MDN page. Here's the link if you'd like to chek it out!

  I noticed that the card is not centered at higher resolutions and the background is not positioned, to fix these things we can do the following:

  body {
  /* Positioning the background */
      background-size: 100% 50vmin;
  
  /* Centering the card */
      display: flex;
      flex-direction: column;
      align-items: center;
      justify-content: center;
      min-height: 100vh;
  }
  .card {
      margin: 0;
  }
  .attribution {
      margin-top: 3rem;
  }
  

  There is an accessibility report due to semantic html elements. Instead of using a div on your footer, try to use the footer element so it'll be easier for a screen reader to see the different parts of the page.

  I see that you are using px instead of rem. I highly recommend you to use rem because if the user has the size of their font smaller or larger your page would be the same. Here are two videos that explain it better:

  1. CSS em and rem
  2. Why You Should Use REM Instead of Pixels

  If you have any questions feel free to send me a message or answer this comment. I hope it helps!

  10

  Marked as helpful
• 

  Hassia Issah•50,390

  @Hassiai

  Posted over 2 years ago

  give the background-size a value of contain instead of cover . Remove the width and height value in the body. Use the colors that were given in the styleguide.md found in the starter folder. Use rem or em as unit for the padding , margin, width and preferably rem for the font-size for more on this watch this https://youtu.be/N5wpD9Ov_To Hope am helpful HAPPY CODING

  10