SASS (Grid & Flexbox)

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Desktop layout looks really great, it is responsive and the mobile state looks great as well.

  Some suggestions would be:

  • First, you should have nest the section on the markup inside the main since it is one of the main content of the page and because it should be inside a landmark element.
  • Avoid using id attribute as a selector in css because it is a bad practice due to css specificity. Use class to target elements.
  • Website-logo should not be inside the nav since it is not being treated as a link, use a tag on it with an appropriate aria-label and use it on the nav.
  • Website-logo img should be using the website's name as the alt like alt="loopstudios". Remember that a website's logo is meaningful so always make sure it uses the proper alt value.
  • Also when using alt attribute, avoid using words that relates to "graphic" such as "logo" and others. An img is already an image/graphic so no need to describe it as one.
  • Do not directly type the wordings as uppercase on the markup, if you do this, screen-reader will read the text letter-by-letter and not by the wordings. Use only the lowercase version to write in the markup and instead use text-transform: uppercase on it.
  • Other images that are used could have use a better alt because what does alt="interactive.jpg" means? Right.
  • When wrapping a text-content do not just use span to wrap it, use meaningful element like a p tag if it just a regular text or heading tag if it is an heading.
  • see all should be using a tag since it will be more like a link on a real site, a page-link where users can "see all" creations.
  • You could use ul on the creations since those are "list" of creations.
  • Also, you could have use a tag inside the creation's title since again, on a real site, it would be a link to view a single creation.

  FOOTER

  • Same for the website-logo it should use the website name as the alt value.
  • Links below the logo could have use nav since those are still your website's navigational links.
  • Each a tag that wraps social media, it should have either aria-label attribute or screen-reader element inside it. The value for whatever method you will use should be the name of the social media like aria-label="facebook" on the facebook link a tag. This way, users will know where this link would take them.
  • img for the social-medias should be hidden since those are only decorative images, so use alt="" and extra aria-hidden="true" on it.

  MOBILE

  • Remove the width: 100vw on the #banner as this causes horizontal scroll.
  • Hamburger menu should be using a button element since it is a control. Again, interactive components uses interactive elements. By using div you are making it not-accessible.

  SUPPOSING BUTTON IS USED

  • The button will be using the method I mentioned using aria-label attribute or screen-reader element inside. The value will describe what does the button do. The value could be aria-label="navigational dropdown menu".
  • The img inside the button should be hidden, use the method I mentioned above.
  • The button should have a default aria-expanded="false" attribute on it. It will be set to true if the user toggles the button.

  Aside from those, great work again on this one.

  Marked as helpful

  1

  ---

  

  Eray•1,410

  @ErayBarslanPosted about 3 years ago

  @pikamart Thank you for the detailed review! I noticed some of them after submitting. Though new things learnt regarding accessbility by your comment.

  For hamburger menu, in this project I wanted go with full CSS, hence I used checkbox instead button. But I guess it would be better to use aria-label and tabindex on the label for accessbility. In the long run I'll be using button with JS for toggle menu. This was more of a challenge when I saw in the brief it could be done without JS.

  Anyways thanks for your time this was really helpful :)

  1