Responsive landing page using Flexbox and CSS Grid

Community feedback

• 

  

  Grace•32,130

  @grace-snow

  Posted over 3 years ago

  Hello

  Really good job overall

  Here are some suggestions:

  • Insert a css reset at the start of your stylesheet
  • Never ever have font sizes in pixels. Use rem (or sometimes em)
  • The features section element should really live within the main element
  • It's better to have an sr-only h2 in that section and use aria-labelledby on the section instead of aria-label. This is partly because aria-label isn't reliably translated into different languages, and partly because that would improve the page structure. At the moment, the parent heading of those feature h3s is actually the "mechanical wireless keyboard" h2. That might be fine, but I think having a "Features" heading would be better.
  • There is horizontal overflow happening on some screen sizes. Overflow-x hidden on the body would probably fix that
  • I don't think you're using the right elements for the buttons. Those should almost certainly be anchor tags as they would take people to another page (trigger navigation).
  • Most important change of all <img src="./assets/shared/logo.svg" alt="logo" class="header__logo">. This means nothing to search engines and assistive tech users. The brand logo (and therefore name) is the most important information on the page. Fix that alt text ;)

  Good luck

  23

  Marked as helpful
• 

  Jimmy Sweeney•350

  @sweenejp

  Posted over 3 years ago

  Hi Gregg,

  Your solution looks pretty close to the design so good work.

  One thing I notice right away is that you could have all your media queries at the end - that way you would just have two media query lines and then put all of the classes that need to update at different sizes within those media queries (instead of having a media query on all of your classes). That might just be a matter of preference though.

  Another solution to loading different image sizes is to create a div with a background-image: url(file-path.jpg) and then update the file path with media queries.

  .info__img-wrapper--brand {
  transform: translateY(13px); 
  }
  

  • don't know that this line is necessary.

  I could be wrong but it doesn't seem that overflow is doing anything. Looks like you achieved the effect of getting images to be off screen with flex-wrap: no-wrap with one of the images and a negative margin with the image. Maybe there was a way to be more consistent here and use no-wrap to achieve the effect for both images?

  Keep up the good work!

  21