css grid

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

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

  Some suggestions would be that:

  • Avoid using height: 100vh on a large container especially the body tag, this limits the element's height based on the remaining screen's height, replace it with min-height: 100vh, this takes full height but lets the element expand if needed. Also, remove the overflow: hidden this makes the content not scrollable, use overflow-x: hidden to hide only for x-axis.
  • The 2 img used for the background needs to use alt="" since it is only a decoration. Also, add an additional aria-hidden="true" attribute on the img. Decorative images needs to be hidden at all times by using these methods.
  • Always have a main element to wrap the main content of your page. For this one, the .container could have used main instead of div.
  • The person's img needs to use the person's name as the alt value like alt="Victor Crest", components like this where the person's image and name is present, use their name as the alt value for the image.
  • A site needs to have 1 h1, since there are not visible text-heading that could use h1, the h1 would be a screen-reader only text. Meaning it will not be visible for sighted users, but users who uses screen-reader will read it. Take a look at Grace's solution on this one inspect the layout and see how she used the h1, copy as well the stylings that she used as the sr-only you will use that a lot.
  • .details could have used ul element since those are "list" of information about the user.
  • The bold text is not supposed to be a heading tag, since it doesn't convey any information about the section. Use p tag on it.

  Aside from those, great work again on this one.

  Marked as helpful

  1

  ---

  

  Vishal Kumar•210

  @LahsivK4070Posted about 3 years ago

  @pikamart thanks for taking your time to review my work. Your suggestions are of great help. Thanks

  0