Sunnyside Landing page using flexbox, grid & vanilla JS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in desktop is good and it responds well, the mobile layout is good as well.

  Regarding your queries:

  1. The reason for the extra white line on the right side is your width: 100vw on the nav element. width: 100vw and width: 100% works different, the 100vw takes all the viewport's width along with the dimension of the supposed scrollbar, that is why there is an extra spacing. You could just remove that width property along with the overflow
  2. You don't really need to use javascript on this part, you could have just use media queries in the css, like when it is one media (min-width: 768px), the hamburger menu is active, then when it is beyond that, just show the desktop layout. You should just use javascript to toggle classes or attributes on that hamburger menu, not using to "show" or "hide" it, let css handle it for you ^^

  Some other suggestions would be:

  • Your nav should be inside the header tag. On this one, I would not include the hero-section in the header. Typically, top most header contains the website logo-link and the navigational-links. So on this one, the preferred structure would be:

  <header>
    <div>
      { The website logo image-link is inside here }
    </div>
    <nav>
       { All the list of links are in here }
    </nav>
  </header>
  

  • Always have a main element on the webpage, main element is used to properly navigate your website. A preferred structure again would be:

  <header />
  <main />
  <footer />
  

  • The arrow-icon on the hero-section should have aria-hidden="true" on the svg element so that it won't be read by assistive tech since it is just decoration.
  • When using heading tags, make sure that the lower level heading are present before the current one. For example, if you use h3 make sure that h1, h2 are present before it. You use heading tags incrementally by 1.
  • section elements could be used as well on this so, especially since there is a testimonial-section.
  • The alt for the person for each testimonial should use their names like img alt="Emily R" since the image itself is meaningful since it is the user who have the testimonial.
  • For each images img element that you use, if the element is just decoration, always have alt="" on them, if you don't include that, a screen reader would just read something from the source-code I think, I forgot what it said, but you should always add that, if the image is meaningful, then add a meaningful alt value.

  FOOTER

  • The links should be inside a ul element since those are list of links, also the ul should be inside a nav since those are still your navigational links.
  • The social media links as well should be wrapped inside ul element.
  • The title for each social media svg should only include the name, remove the word logo, when using title or alt do not add words that relates to "graphic" like "image, logo, icon" assistive tech will handle those for you.

  MOBILE

  • The hamburger menu should be a button and not a tag since it is not a link, rather a control-button for the menu-dropdown.
  • The hamburger toggler also should have aria-label="menu dropdown toggler", this will make sure that screen reader users would know what this button does. Also it should have aria-expanded="false" as the default and you should change it via javascript. You might want to search about it, or you could reply in here and maybe I can create a simple snippet.
  • The svg inside the menu-toggler should have aria-hidden="true" since that is just an icon for the dropdown.

  Aside from those, really great job on this one.

  Marked as helpful

  0
• 

  Phil•185

  @PhilJGPosted about 3 years ago

  That worked great thanks again Raymart!

  0
• 

  Phil•185

  @PhilJGPosted about 3 years ago

  Hey Raymart,

  Thanks so much for your feedback.

  I made your adjustments and thank you for enlightening me on best practices for proper html arrangement my knowledge is basically zero and this was the first time I was even aware of aria. I made all the adjustments you suggested except the one about adding aria false to my javascript. I didn`t really understand the concept or what you meant I will have to do more research.

  Also, I took your suggestion of removing overflow & and the width from the navbar. It worked, however it messed up my navbar on mobile by squishing it to the left side. Any ideas on how to remedy this issue without causing more overflow?

  And many thanks for your feedback it helped me greatly :)

  Phil

  0

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @PhilJG Hey, great that it helped you with it.

  It is fine that arias are new for you and it is okay that if you don't rush it by now, but it is good that you are aware of it^

  For the navbar, just only remove the width: 100vw on desktop layout, then on mobile layout, just bring back width: 100% and will work just fine, you also have this on mobile layout, the 100%

  Marked as helpful

  1