sunnyside agency

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. The desktop layout looks fine for me, just that the hero-section is bit to taller, the text-alignment is using center instead of left and the white-spaces on the testimonial section could be reduce. For responsiveness, if you go at point 770px, you will notice that the site's content is being by the screen causing a horizontal scrollbar and the text of the site starts to get squished, the images doesn't respond and alignment is still not going well.

  David Turner already gave a feedback on this one, just going to add some suggestions as well:

  • It would be great to have this markup:

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

  This way, all content of your page will be inside their own respective landmark element. Remember that for the primary header, nest only the topmost part of the site which are the site-logo, navlinks and leave the hero-section inside the main tag because you want the header to be reusable for other pages, so only include what is needed on that.

  • The site-logo could be remove from the nav since it is not being treated as a link. If you insist on doing so, it would be nice to wrap the img by a tag and make sure to properly use an aria-label or sr-only text that would describe where the link would take the user.
  • Also, on the header tag's nav, you can add aria-label="primary" so that it will be unique since a another nav could be used inside the footer tag.
  • Those 4 links in the header could be wrap inside a ul tag since those are "list" of links.
  • Also, I saw a usage of height: 100vw on the .h-screen selector. Avoid using height: 100vh as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
  • Also, I would prefer to use rem value to give the height on the hero-section instead of vh unit so that it will stay consistent for all user. Try to inspect the layout in dev tools at the bottom, the hero-section changes a lot when you resize the screen's height.
  • The arrow-image on the hero-section is only a decorative image. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
  • For this one, just use a single h1 since you don't want it to be repetitive on the page. Use a single h1 only and change those into other heading tags.
  • It would be nice to use text-align: left as well on each of the section after the hero so that it will look like the design.
  • Those learn-more are better using a tag rather than button. On a real site, those would be page links and not like a control.
  • For this challenge, I would only use alt attributes on the site-logo and each of the person's on the testimonial section and the rest would be hidden using the method above since they are all decorative images.
  • For the cherry and the orange section, you can instead use those images as background-image for each container so that they will be easier to handle.
  • On the testimonial section, it would be nice to adjust the white-spaces on that part because right now, they are too much:>
  • For each testimonial, you can use this markup below so that it will be easier for the user to traverse the information using blockquote on a screen-reader:

  <figure>
    <img src="" alt={person name}>
    <blockquote>
      <p> {qoute in here}</p>
    </blockquote>
    <figcaption>
      person name
      <p>
        person role
      </p>
    </figcaption>
  </figure>
  

  FOOTER

  • The background-color could be lighter so that it will match the footer's design.
  • Those 3 links after the site-logo could be wrapped inside a ul because again, they are "list" of links. Also, the ul on this could be wrapper inside a nav since they are still your site's navigation links. The nav should have aria-label="footer" so that it will be unique.
  • Those social-media links could be inside a ul element since those are "list" of links.
  • Each a tag that wraps the social-media icon should have either aria-label attribute or sr-only text inside it, defining where the link would take them. For example, you should use facebook as the value if the link would take the user to facebook.
  • Social-media image should be hidden since it is only a decorative image so use alt="" and aria-hidden="true".

  MOBILE

  • I noticed on the markup, you duplicated your navlinks for the mobile state which you don't really need. You just need to style the original navlinks to adapt for mobile state and desktop state. This way, you are reducing repetitive element on your markup.
  • Hamburger menu should be using a button since it is an interactive component. When creating interactive component, make sure that you are using interactive element so that it will be accessible for other users.

  SUPPOSING BUTTON IS USED

  • The hamburger button should have a default attribute of aria-expanded="false" and it will be set to true when the users toggles it and vice-versa.

  • The hamburger button should have either aria-label attribute or sr-only text inside it which defines what the button does. You could use aria-label="navigation dropdown menu"

  • The svg inside the hamburger-menu should have been hidden since it is only a decorative image so use aria-hidden="true" on it.

  • Also, the placement of your hamburger and the dropdown is incorrect. The dropdown should be placed after the hamburger so that after the user toggles it, the next focus is on the dropdown.

  • Lastly, in general, adjusting the styling on the site and addressing the responsiveness issue:>

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  Nnenna Miriam Udefi•300

  @Nnenna-udefiPosted almost 3 years ago

  @pikapikamart I'll try my best to correct the mistakes. Thank you

  1
• 

  P

  David Turner•4,150

  @brodiewebdtPosted almost 3 years ago

  This looks good, but it needs some work at Ipad resolution. 768px. The text isn't displaying properly. The mobile menu can use some space on the right hand side as it overflows the container. The menu works fine though. This was a tough project to do.

  Wrap all you content in a Main tag and change the H3 to H2. You don't want to skip heading levels. You may have to re-style the get it to look like the design. These will clear the accessibility warnings.

  Download AXE DevTools and you can clear accessibility warnings while you code. https://www.deque.com/axe/devtools/

  Hope this helps.

  0

  ---

  

  Nnenna Miriam Udefi•300

  @Nnenna-udefiPosted almost 3 years ago

  @brodiewebdt Yes, this was really helpful. Thank you

  0