first-project-responsive-order-summary-using-grid, only html & scss

Community feedback

• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @NovidicusTitan 👋🏻

  I've got some suggestions to help you fix the accessibility and HTML issues.

  • In your markup, <div class="">...</div> should be <main class="">...</main> and <div class="attribution">...</div> should be <footer class="attribution">...</footer>.
  • Next, you cannot nest button inside <a> tag or vice versa. In this case you have to choose between one of them.
  • Next, I suggest using <h2> or <p> instead of <h4>, because headings in HTML have to increase gradually, such as h1, h2, h3….. These will fix the accessibility and HTML issues. Don't forget to generate a new repot once you fix the issues.
  • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here.
  • Hero image should have a display: block;, it removes the line underneath the image. If you want to know what's causing it, check out the 3rd section of this video.
  • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.
  • Lastly, I won't go into many details about resetting CSS, but I'll leave this cool article here, which will make more sense than my brief explanation.

  I hope this was helpful 👨🏻‍💻 all in all, you did a good job for your first project, nicely done. Cheers 👾

  Marked as helpful

  0

  ---

  

  Novidicus Titan•60

  @NovidicusTitanPosted about 3 years ago

  @kens-visuals Thank you for taking a look at my code, i made the changes you sugested

  0

  

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  @NovidicusTitan no problem 😇

  0
• 

  Saurav Singh Khekhaliya•190

  @sskhekhaliyaPosted about 3 years ago

  hey, I just looked at your site, there are some suggestions:

  1. There is no need to using border-radius: 20px 20px 0px 0px; for image, instead of it just apply overflow: hidden; to .container. So it will hide the corners of images also.
  2. It will be good to give a little padding-bottom to the card.
  3. Card's margin-top:40px; didn't make it vertically centered and if someone will see your site on the big screen then the card is not able to be vertically centered. So for it, you can use display: flex; for body and then set its direction to column (flex-direction: column;) and give a height to the body 100vh.

  Marked as helpful

  0

  ---

  

  Novidicus Titan•60

  @NovidicusTitanPosted about 3 years ago

  @sskhekhaliya Thank you, i have fixed the border padding and centered it vertically, learned something about flex.

  1