Responsive order summary component using flexbox

Community feedback

• 

  Carlos•1,110

  @Carlos-A-PPosted about 3 years ago

  Hey Norrico,

  • There seem to be fixed widths throughout your CSS for the desktop view which isn't responsive friendly. I'd suggest using max-width instead.

  • As for the accessibility issues, instead of using a <section> tag you should use a <main> tag

  • You can remove display flex and fixed width/ height from your body tag, then in your <section> tag use:

  .container {
      width: 100vw;
      height: 100vh;
      display: flex;
      justify-content: center;
      align-items: center;
      background-image: url(./images/pattern-background-desktop.svg);
      background-size: 100% auto;
      background-repeat: no-repeat;
      /* box-shadow: 0px 16px 28px 0px rgb(0 0 0 / 20%); */
  }
  

  • This actually made it more responsive aha.

  Hope this helps and good luck coding!

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @Carlos-A-P Hey, great tip on this one and really nice seeing others giving feedback. Just to change something, do not use/suggest using height: 100vh since this limit's an element's height based on the remaining viewport's height, min-height: 100vh is the preferred since it allows the element to expand if needed.

  width: 100vw as well is not great since this adds a extra scroll vertically since this doesn't account the horizontal-scrollbar's size on the right part. 100% would be better.

  It is still really great seeing lots giving feedbacks, awesome as well on this!

  1

  

  Carlos•1,110

  @Carlos-A-PPosted about 3 years ago

  @pikamart thanks for catching that! I had a habit of using viewport units for smaller projects but I'm glad I'm always learning ways to improve. Thanks again!

  1
• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout looks great in general.

  Other already gave great feedbacks on this, just going to add some small suggestions as well:

  • Avoid using height: 100vh on a large container like the body tag as this limits the element's height based on the remaining screen's height. Instead use min-height: 100vh, this takes full height but lets the element expand if needed. Same goes for other element as well.
  • Instead of using width: 1440px; on the body use max-width: 1440px this way the body won't have a fixed width which causes the horizontal scrollbar at the bottom.
  • .container should be using full width so use width: 100%.
  • annual-plan should be a heading tag since it gives information on what the section would contain.

  Aside from those, great work again on this one.

  Marked as helpful

  0
• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @norrico31 👋🏻

  One small suggestions from me to improve the accessibility.

  • For the music icon, add aria-hidden="true”, because it's for decorative purposes. You can read more about aria-hidden here.

  I hope this was helpful 👨🏻‍💻 really good job for the first project, the next thing would be to make it responsive, as already suggest above. Cheers 👾

  Marked as helpful

  0