Responsive Order-Summary-Card

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. Site in general looks great and responsive.

  Stefan already gave a feedback on this, just going to add some suggestions as well:

  • Instead of creating a .bg-image , use the background-image directly on the body tag. I think you did this since the image keeps repeating right? To prevent this, just simple add background-repeat: no-repeat on the body tag as well.

  body {
    background-image: put in here;
    background-repeat: no-repeat;
    background-size: 100%;
  }
  

  • Using margin to center an element on both axis is not consistent enough. Instead what you can do is, on the body tag add these stylings:

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

  Though I am implying first that the .bg-image element is already removed before applying the stylings.

  • Always have a main element to wrap the main content of the site. For this use main tag on the .container selector.
  • The illustration could use an extra aria-hidden="true" attribute on it so that it will be totally hidden for screen-reader users. The same with the music-icon.
  • The order-summary could use an h1 since an h1 is needed in a page. Also, when using a heading tag, make sure you are not skipping a level. If you use h4 then make sure that h1, h2, h3 are all present.
  • Good choice of making annual plan a heading tag, but wrong level. Use h2 on it.
  • Do not remove the outline styling. If you did, always include a visual-indicator on the :focus-visible for those interactive elements like the button a tag and others.
  • Lastly, the .attribution should use a footer tag so that it will be its own landmark.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  AhmedFawzi•70

  @AhmedFawzi44Posted almost 3 years ago

  @pikamart Thanks for the Advices! ❤ i will make few changes in the near future on some of my designs so THANKS again for this feedback 🤍

  1
• 

  Stefan•180

  @st3wnPosted almost 3 years ago

  Hi man,

  I recommend to you to put the full box in one container like.

  <div class="container"> In it you put everything of the summary box. </div>

  0