order-summary-component

Community feedback

• 

  Adriano•38,060

  @AdrianoEscarabotePosted about 2 years ago

  Hi Luke Janssen, how are you?

  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
  • images must have alt text unless it is a decorative image, for any decorative image each IMG tag must have empty alt="" and add aria-hidden="true" attributes to make all the assistive technologies of the Web, as screen reader. Learn the differences between decorative/meaningless images vs important content.
  • 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;
  }
  

  To prevent the background image from breaking at higher resolutions, we can prevent this in two different ways:

  1. Add a background-repeat: repeat-x;, the image will repeat on the horizontal axis, preventing it from breaking.

  2. Add a background-size: 100% 50vmin;, the 50vmin will set its height as the page target, and 100% will make it stretch on the horizontal axis.

  Feel free to choose one of the two!

  The rest is great!

  I hope it helps... 👍

  Marked as helpful

  1

  ---

  

  Luke Janssen•210

  @lukejansPosted about 2 years ago

  @AdrianoEscarabote Thanks so much for the help on this one! I was in the process of going with flexbox on the main container which has massively helped! I appreciate the feedback massively and have just made this more accessible for screen readers and fixed the background!

  0

  

  Adriano•38,060

  @AdrianoEscarabotePosted about 2 years ago

  @lukejans happy coding!!

  0
• 

  Adeola Ganiu•1,320

  @DeolabestPosted about 2 years ago

  Hey @lukejans, Congratulations on completing this challenge!

  Here is my feedback:

  • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

  • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

  Keep doing a good job!

  Marked as helpful

  1

  ---

  

  Luke Janssen•210

  @lukejansPosted about 2 years ago

  @Deolabest Thanks! I appreciate the feedback! I just updated my div with a class of container to the main tag. I also got the favicon to work by removing the deployment from a second branch and restricting my directories! Any more feedback would be appreciated! still need to update font px to rem though!

  1