Order summary component challenge hub

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, really nice work on this one. The overall layout looks fine, just the background-image-wave to be placed in the center properly is needed.

  David Turner already gave really great feedback on this one, just going to add some suggestions as well and don't mind me re-iterating some of David's point:

  • Avoid using height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
  • For the .white container, for the background-image to be placed properly, you can set this stylings:

  background-repeat: no-repeat;
  background-position: center top;
  background-size: 100%;
  

  Also, I second to what David said about using this styling on the body tag instead on the .white so that it will fill up the whole screen.

  • It would be great to have this markup:

  <main /> # contains the main content
  <footer /> # contains the attribution
  

  This way, all content of your page will be inside their own respective landmark element.

  • Adding an extra aria-hidden="true" attribute on each of the img tag that you uses with alt="". This will ensure that screen-reader will ignore it when traversing images in the dom.
  • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
  • The annual plan text could have used a heading tag like an h2 on this since it gives information on what the section would contain, hence the pricing plan.
  • proceed to payment is an interactive component so using h4 is not a great choice. Use either button or a tag on this one.
  • Lastly, changing some width property into using max-width so that the layout will scale properly especially when you go to small screen size like 400px below.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Hossam Gouda•130

  @HossamGoudaPosted almost 3 years ago

  @pikapikamart Thank you for your awesome and organized reply it made a lot of stuff clear for me.

  0
• 

  P

  David Turner•4,150

  @brodiewebdtPosted almost 3 years ago

  Move the background-image styles from the white div to the body tag. Add the light blue color to the body before the image styles to cover the white. After you do that remove the white div as the styles are not needed.

  Add display:grid; place-items: center; min-height:100vh to center the card in the screen.

  Your media query is not working because you have width: 375px instead of min-width or max-width. You should remove it because it breaks your layout. Also you have a set width of 400px on some of your divs so it won't resize below that.

  Give your card some left and right padding as the text is right up against the edges of the screen.

  Let me know if you need any more help.

  Marked as helpful

  0

  ---

  

  Hossam Gouda•130

  @HossamGoudaPosted almost 3 years ago

  @brodiewebdt Thanks so much for your reply it is really helped me.

  0

  

  P

  David Turner•4,150

  @brodiewebdtPosted almost 3 years ago

  @HossamGouda Your welcome. Glad I could help.

  Marked as helpful

  0
• 

  Hossam Gouda•130

  @HossamGoudaPosted almost 3 years ago

  Now I made it much better but I still want to understand why the hero image doesn't fill the edges for the div element.it is leaving behind around 3px of white background

  0