Responsive landing pages with vanila js, scss and BEM

Community feedback

• 

  Haziq•320

  @zyq-mPosted about 3 years ago

  Hi Raymart Pamplona, thanks for reviewing my codes. I appreciate it. Thanks also for the suggestion. I've learned a lot new things from you. Thank you again!!

  0
• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, really great work on this one. Layout in desktop looks really great and the looks really is close to the design. It is responsive and the mobile state looks great as well.

  Some suggestions would be:

  • The website-logo-link shouldn't be inside the nav since it is not your navigational links, those 4 links on its right side are the ones that should be inside.
  • The a tag that wraps the website-logo should have aria-label attribute or an sr-only text inside it, defining where this link would take them, homepage could be used as the value, since the a tag uses "/" route, which is the home or root route.
  • The website-logo should only be using alt="sunnyside" lose the word "logo". When using alt attribute, avoid adding words that relates to "graphic" such as "logo, icon, image.." it is already an image, no need to describe it as one.
  • The arrow-icon-image should be using alt="" empty, and aria-hidden="true" as extra attribute. If an image acts as a decoration, you use those two that I mentioned, but if the image adds content to the site, then use a meaningful alt attribute.
  • For some reason the text client testimonial is not appearing.
  • Since you use article on the testimonial section, you should include the person's image inside it, since those are just one whole component.

  FOOTER

  • Website-logo- svg should have title element inside the svg, that title element will be using a sr-only class to make it not visible. When using img alt is used, but when using svg title is used but the title should have sr-only class, if you want the text not be visible.
  • The 3 links could be inside nav element as well, since those are still your navigational links, using ul as well inside the nav to wrap those will be really great.
  • For the social media section, you should use ul element on those, since those are "list" of social-media-links and each svg should be wrapped inside a tag.
  • Each a tag that wraps the svg should have either aria-label or an sr-only text inside the a tag, describing where the link would take them. As an example:

  Using aria-label
  <a href="facebook.com" aria-label="facebook">
  {  svg inside here}
  </a>
  
  sr-only
  <a href="facebook.com>
    <span class="sr-only"> facebook </span>
    { svg inside here }
  </a>
  
  .sr-only {
    border: 0;
    clip: rect(0 0 0 0);
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    position: absolute;
    width: 1px;
  }
  

  Using the sr-only is the better choice, since the text is translatable for other languages.

  MOBILE

  • Using img for the hamburger is not really great. When creating interactive component, use interactive element. For this one , you should be using button.

  SUPPOSING BUTTON IS USED

  • The button should have either aria-label attribute or an sr-only text inside it, describing what the button does, you could use navigation menu dropdown as a text.
  • The button should also have aria-expanded="false" as a default attribute, the value will be changed to "true" by javascript, if the toggle is clicked.You can see simple snippet of mine, check the javascript, this is just a simple example for the aria-expanded.

  Aside from those, really great work again on this one.

  0