Order Summary Challenge Solution

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

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

  Fidel Lim already pointed great tips on this one, just going to add some suggestions as well:

  • It would be great to have a base styling with these properties

  html {
    box-sizing: border-box;
    font-size: 100%;
  }
  *,
  *::before,
  *::after {
    box-sizing: inherit;
  }
  

  This way, handling an element's size will be lot easier because of the box-sizing: border-box. You can search up some more of how this works.

  • Always have a main element to wrap the main content of your page. On this one, the section should be using the main instead of div.
  • Also, using margin to center the layout is not consistent enough. To make the layout center always and properly, on the current markup, first remove the margin on the section and on the body tag add:

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

  It might seem a lot but you will find yourself using this kind of stylings as well especially when doing a site with just small content.

  • Vector and the music-icon should be hidden since they are only decoration. Decorative image must be hidden at all times by using alt="" and extra aria-hidden="true" attribute on the img tag.
  • Also when using alt attribute, avoid using words that relates to "graphic" such as "illustration" and others. An img is already an image/graphic so no need to describe it as one.
  • Using aside is not really suited on this one, you could use div or section instead of it.
  • annual-plan should be a heading tag since it gives information on what the section would/could ontain.

  Aside from those, great work. Also if you have any queries, just let me know and see if I could help with it.

  Marked as helpful

  1

  ---

  

  Nabil-Y•150

  @Nabil-YPosted about 3 years ago

  Hi @pikamart,

  Thanks a lot for all your suggestions, it is much to remember bur your thorough review is really helpful and I'll try to apply it from now on.

  As you advised I've changed the "section" div to "main" and it seems it cleared the main issues on my automatic report.

  There are just two things I didn't understand very well.

  • Why do you specify a min height of 100vh when centering the content?
  • Why hide the decorative element and remove the alt text? I've tried what you said but aria-hidden="true" doesn't seem to hide anything on the page and isn't it bad for accessibility to remove alt text?

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @Nabil-Y Hey, glad to be back on this one and great that you find the suggestion useful^^

  • When you are centering a content inside a flexbox or grid, when you want to center the content on the x-axis, you use justify-content: center but making sure the element have enough width to center the content. But when you are centering for y-axis, vertically, your flexbox first should have enough height to center the content via align-items: center that is why I suggested to add min-height: 100vh so that your flexbox will have a minimum height based on the total height of your screen. This way it will have enough height to center it.

  • Yes, always hide decorative images. On a website, you only want to show users meaningful images. Now what I said might be confusing and new to you so here how it works:

  • When an image is meaningful, use a descriptive alt value.

  • If the image is only decoration, hide it. When I say or when you encounter people saying, hide the image, you don't take it literally hehe. Hide it for screen-reader users, to hide those images, you use alt="" see that it is empty. When a screen-reader encounters an img that has an empty alt it discarded this image and does not read.

  • For the aria-hidden="true", you add this to a decorative image along with the alt="". Now, aria-hidden="true" does not hide the image visually, it works the same with an empty alt but for other screen-readers, using alt="' alone does not hide, that is why adding aria-hidden="true" completely hides the img for screen-reader users.

  Marked as helpful

  1

  

  Nabil-Y•150

  @Nabil-YPosted about 3 years ago

  @pikamart Thanks a lot for your thorough explanations, I understand much better now!

  I've updated my code and included everything you suggested and even added some animations for the hover states.

  Have a nice day :)

  1
• 

  Fidel Lim•2,775

  @fidellimPosted about 3 years ago

  Hi @Nabil-Y,

  Great job finishing the project! It looks great on desktop and mobile devices. Some suggestions I would like to share are of the following:

  • you can replace this div (class: attribution) with a footer tag. This will fix one of the issues with your report.

  <div class="attribution">
      Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>. 
      Coded by <a href="#">Nabil-Y</a>.
    </div>
  

  • if you have a section tag, it is advisable to have headings between h2 and h6.

  I hope this helps :)

  Marked as helpful

  1

  ---

  

  Nabil-Y•150

  @Nabil-YPosted about 3 years ago

  Hi @fidellim,

  Thanks to your feedback, it helped me get 0 issues in the report!

  I thought about changing the h1 to h2 but I've changed the "section" to "main" and kept the h1 heading as the report was saying it is better for accessibility to have at least one h1 in the page.

  I still got a long way to go but I feel I have learned a lot doing this. Thank you!

  0