Responsive Landing Page using CSS Grid and Flexbox

Community feedback

• 

  Dusan Brankov•860

  @dusan-bPosted about 2 years ago

  Hello James,

  I just looked at your code and I like your approach towards web accessibility and semantic HTML. However, there are some changes I would consider.

  I see you've used the <a> element for your menu button. Instead you should use the <button> element, since its purpose is to perform an action (opening and closing the menu). Anchor elements are only there to navigate you to another page. Otherwise keyboard users wouldn't be able to go through the menu when it's hidden, and visually impaired users would get confused when using a screen reader.

  Another thing is, it's unfortunately not valid (yet) to specify the aria-hidden attribute on <picture> elements. A reasonable solution would be to leave the alt attribute empty. It's acutally even better this way, since this method has been established a lot longer and will be accepted by more assistive technologies. But keep in mind, this is only applicable to decorative images.

  Also, the <main> element should contain all the content which is unique to the page, in this case everything except header and footer. Otherwise you would keep getting the warning "All page content should be contained by landmarks".

  I hope this helps. Keep up the good work!

  Marked as helpful

  1

  ---

  

  James•160

  @jamescheinPosted about 2 years ago

  @dusan-b

  Thank you so much for taking the time to review my site and provide this helpful feedback.

  I actually spent time during the challenge looking at the differences between 'button' and 'a' tag regarding the 'read more' link to find which tag was appropriate in the context and didn't even think to then apply this knowledge to the open/close menu trigger - oops ;). Thank you for pointing this out.

  Now also looking at BEM naming which is new to me.

  James :)

  1