Sunny Side Up with Next.js/React/TailwindCSS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks fine, it is responsive and the mobile state looks great.

  Some suggestions would be:

  • On this one, It would be better to use a markup like this:

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

  Since using header on this one does make sense right and making it as a landmark on its own row.

  • I would use the svg for the website-logo instead of h1. h1 is much better on the hero-text.
  • Wrap those supposed navlinks inside nav. Also, why are you not making those links a tags? interactive components needs to have interactive elements.
  • 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.
  • Avoid using multiple h1 on a page, use only at least 1 per page so change those into other heading tags.
  • Avoid using height: 100vh in general as this is limits the element's height, use min-height: 100vh instead or use rem to be consistent.
  • The arrow-icon should be hidden since it is only a decorative image. Decorative image must be hidden at all times for screen-reader users by using alt="" and extra aria-hidden="true" attribute on the img tag. Only add a descriptive alt when the image is meaningful and adds content to the site.
  • Those other images as well, on this one, I only find the testimonial people images and the logo to be meaningful and the rest are just decoration.
  • learn more should be a link.
  • When using alt attribute, avoid using words that relates to "graphic" such as "image" and others. An img is already an image/graphic so no need to describe it as one.
  • Have a look at blockquote element, this will help you use a proper markup on a testimonial card.

  FOOTER

  • Same, thos navlinks should be using a tag and within nav since those are still your website's navigational links:

  <nav>
    <ul>
      list of links
    </ul>
  </nav>
  

  • Social media links could be inside ul since those are "list" of 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.
  • Each svg inside the social media link should be hidden since they are only decoration so use aria-hidden="true" attribute on them.

  MOBILE

  • 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 img inside the hamburger should be hidden, use the method I mentioned above.
  • 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 button should have a default aria-expanded="false" attribute on it. It will be set to true if the user toggles the button.
  • Using only opacity to hide an element does not really hide the element completely, add a visibility property so that it will be hidden or visible.

  Right now, there are lots of improper usage of html elements in here. It is great to learn libraries(react) and framework(nextjs) but before doing so, make sure that your fundamentals of html and css first is great. At the end, you don't want to create a website that is not accessible right.

  But still, great job on this one.

  Marked as helpful

  1

  ---

  

  MikevPeeren•2,100

  @MikevPeerenPosted about 3 years ago

  @pikamart Hey Pikamart,

  Thanks allot for the feedback, I have changed my project to include all the feedback. Accessibility is something that falls short for me often as it is more an after thought, thanks for reminding me that I need to keep a focus on it in all my projects.

  1
• 

  Anton Lovrić•20

  @antonlovricPosted about 3 years ago

  Hello Mike! First of all the site looks really cool! :) I'd love to give feedback on the code but I haven't yet used React, Next.js or Tailwind for that matter, so I will just comment on one or two things. Maybe I didn't understand the task well, but shouldn't the active states (like the underline under the "Learn more" links) appear only when you hover/click them? You did a good job implementing them but they are here all the time, instead of just when the user interacts with the elements. Also, I would maybe shorten the mobile menu animation. But that is just personal preference Good job nonetheless! :)

  0

  ---

  

  MikevPeeren•2,100

  @MikevPeerenPosted about 3 years ago

  Hey @antonlovric,

  Thanks for the feedback, the learn more has a default state regarding the color and that color changes in opacity when on hover making it more brighter, the design also has this.

  0