Order Summary Component

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in general looks great and it is responsive.

  Some suggestions would be:

  • Right now, your html doesn't have width or content. Try to inspect the layout, hover on the html tag, you will notice it don't have a size right, it is because the body tag is out of the flow since you use position: absolute on it. This is not advisable to use especially on a large element. You should remove this property.
  • Also on the body tag, remove the height and width since you don't need it. block elements by default uses 100% of the width and avoid using height: 100vh as this limits the element's height. Use min-height: 100vh instead since this is much flexible.
  • Those 2 img that is used as a background, it would be better if they were set at the body tags background property.
  • Since you are using margin to center the #container element right, instead of this, remove all margin on it and on the body tag add these stylings:

      align-items: center;
      display: flex;
      flex-direction: column;
      justify-content: center;
      min-height: 100vh;
  

  This way, your content is being centered properly. Take note of the flex-direction I set it to column since there are 2 elements that are inside the flow inside the body tag.

  • Add an extra aria-hidden="true" attribute on the vector img to make it totally hidden.
  • The order summary could have use a heading tag like h1 for now. Also remember that h1 is needed for every page, a single h1.
  • Music icon could use the aria-hidden="true" as well.
  • annual plan should be a heading since it gives overview on what the section would contain. Also when wrapping text-content, do not just use div to wrap it, use meaningful element like p tag for regular text.

  Aside from those, great work on this one. If you have queries, just drop it here okay.

  1

  ---

  

  Salomon Levi•60

  @saloleviPosted about 3 years ago

  @pikamart Thank you so much, this is very helpful, I struggled while inspecting since the body for some reason kept occupying just the space of the "content" and I didn't know how to fix that. I'll keep in mind every tip you gave, and I thank you for taking your time to review my code. Just one quick question, what does the aria-hidden property do? And should I add them on the hero and music images?

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @salolevi An aria-hidden property or aria-hidden="true" hides an element so that it won't be picked up by other assistive tech like a screen-reader/voice over.

  You typically add them in combine with alt attribute on img tag.

  When using svg since there are no alt right, to hide it you will need to use aria-hidden="true".

  Decorative images should always be hidden at all times using those methods^^

  1

  

  Salomon Levi•60

  @saloleviPosted about 3 years ago

  @pikamart Thank you!

  0