Multipage Website, SEMANTIC html, scss, vanillaJS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, really awesome job on this one. This is the first one I think I saw a solution for this challenge, really really awesome design. Layout in desktop looks great it is really responsive and the mobile state looks really great as well.

  I don't know if I can review the other pages, but here are for the homepage:

  • First, there is a bug on your console, try to fix that because if you open dev tools while refreshing the site, it won't since debugger won't allow it until the bug is fixed.
  • Don't use the h1 to wrap the website-logo-link, just let the a tag or a div to wrap it.
  • You could wrap the website-link inside the nav since it acts as the homepage-link for the site and there are no other links that takes the user to the home other than it.
  • Adjust the :focus-visible state for the button tags since it is hard to tell where I am at when navigating through them.
  • It would be great to have an aria-current="page" attribute to whatever page you are in so that users will know what page they are at.
  • For the hero-sections svg you don't use aria-label to it, to add like an alt text to it, you would need to create an title element inside the svg describing the looks of the svg. Also using smartphone is not descriptive enough, since the image is using the payapi, you might want to include that keyword to it.
  • fieldset is not need inside the form since there is only 1 input also since you are not adding a legend inside it, you don't really need that on this one.
  • Adding a custom validation on the input would be really great.
  • input lacks a label associated with it, since there is no visible text, the label will only be a screen-reader label associated to it.
  • button should have a type="submit" to it, always be explicit on how your button will act especially when it is inside a form. Since by default button inside a form turns to type="submit" and imagine you have a close-button inside the form without using type= button".
  • Same goes for the company svg's use title element inside to describe what is the name of the company and not aria-label. Also you can use ul on those since they are "list" of links.
  • about us on the section with the company logos should be using a tag and not button.
  • The 3 icons-img before the cta are only decoration. Those decorative images on the site could have use an extra aria-hidden="true" attribute so that they will be totally hidden alongside with the alt=""
  • Same with the form inside the cta take note of the suggested above in the first form.

  FOOTER

  • Website-logo-link a tag lacks aria-label or screen-reader only text on it. Describe where this link would take the user.
  • Website-logo svg should use the screen-reader only title element inside it and the text-content will be the name of the site, pay api.
  • Same as well, you can nest the website-link inside the nav but those social media links should not be since they are not your website's navigational links.
  • Those social media should be sitting on their own parent container not with the nav but using ul on it is great.
  • 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, remove the aria-label.

  MOBILE

  • Hamburger menu should be using a button element since it is a control. 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 button should have a default aria-expanded="false" attribute on it. It will be set to true if the user toggles the button.
  • The placement for the button and the dropdown is incorrect. The dropdown should be placed "after" the button so that after the user has toggled the button the focus will be next on the dropdown because on the current setup, you would need to backward navigate since the button is after the dropdown.

  For now, homepage only. But again, this is really great and really awesome seeing this challenge.

  Marked as helpful

  1

  ---

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart hi Raymart, thank you so much for taking the time and giving me such a high valuable and precise feedback!😍 This challenge brought me to a new level, but your suggestions even more!🚀

  I'll fix all these by tomorrow and let you know once I'm finish, so that you see that I take your feedback very seriously😊

  1

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart hi Raymart, issues are (hopefully) fixed now.

  4th bullet point (adjusting :focus-visible state on buttons): What do you mean by hard to tell where you are while navigate through? When I navigate through the website, the button's background-color (and color on all secondary buttons) change, just as in the design. Or do I miss something?

  On line 16 of index.html, I have the attribute <title> and <aria-current = true>. Do I really need both, or can I stick with just aria-current? What is best practice on that?

  Just a note: Third bullet point (wrapping the logo within the nav): You're definitely right here, but since it needs too many adjustments on other parts, I'll let this one as it is, BUT I'll consider this on future projects.

  Thanks again, I am so excited to use these new skills for future projects right away!

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @daniel-hennig Hey, glad that you find the suggestion useful^^

  • About the :focus-visible, yep you got the design right but it is still somewhat hard to tell when I am at right now, well for me since my eyesight sometimes is not that great when looking at screens but it is great when I don't look at great, this eyes. That is why on the :focus-visible state of the button you can make a customized outline with outline-offset, example is

    element:focus-visible {
      outline: 2px dashed black;
      outline-offset: 3px;
  }
  

  This way, it will be easy to see when I tab through the page since there is an outline that is much visible.

  • Great, that aria-current="page" will work just fine, stick to using that whenever you have a multi-page site. This way, when screen-reader users navigated through the navlinks, they will be informed that they are in this specific page of the site.

  • Okay, but make sure to add the website-logo-link in the nav if there are no visible home link, but if you have like a home link, then you don't need to add the website-logo-link inside the nav.

  Again, awesome implementation of the feedback again^

  Marked as helpful

  1

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart Thank you so much, Raymart, these are really useful suggestions! I updated the Website and GitHub-code with the :focus-visible outlines. All your suggestions are already incorporated into the new project, which will come very soon.🙏

  1