Github Search App with react and sass

Community feedback

• 

  Raymart Pamplona•16,040

  @pikapikamartPosted over 3 years ago

  Hey, awesome work on this one. Layout in general looks great and it is responsive.

  Jimmy Sweeney already gave feedback on this one, just going to add some suggestions as well:

  • I would make the devfinder use h1 rather than h2 at the moment or just maybe use the svg version of the logo, is there a svg on this?
  • Your theme toggler works just fine, but it is limited for mouse users only because are not using proper element on it. Remember that interactive component needs to use interactive element, using div makes it not-accessible. The proper markup for the toggle would be to use 2 radio buttons that are inside a fieldset along with legend either using "light" or "dark" as the text-content. I don't have a solution on this one though, but have a look at my rest api country solution check on how I use markup on the color theme toggler.
  • Also the img being use on the toggle should be hidden since it is only decoration. 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.
  • Nest the search-bar inside a form so that it will be clear on the markup or site.
  • img on the search-bar is decorative, hide it using the method above.
  • Your aria-describedBy usage is not correct at the moment. If the input is invalid, use the error-message id as the value for the aria.

  if ( input is wrong )
    input.setAttribute("aria-invalid", "true");
    input.setAttribute("aria-describedBy", id of the error-message);
  else 
    input.removeAttribute("aria-invalid");
    input.removeAttribute("aria-describedBy");
  

  • Another idea to implement to further to make it a accessible, is to have an aria-live element that will announce either if there is a user found of if there is not or an invalid entry. This way, users will be informed right away on what happened after submitting the form.
  • User's image should be using the user's name as the value and not profile since it does not give any information at all. It could be like profile of many things.
  • Your .user-stats could have used ul since those are "list" of informations about the user or maybe a table? Hmmm
  • When wrapping a text-content do not just use div to wrap it, use meaningful element like a p tag if it just a regular text or heading tag if it is an heading.
  • Your .user-links could have used ul as well, again, "list" of information. But instead of just using ul, nest it inside a section same with the .user-stats

  <section>
    <h3 class="sr-only">User profile information</h3>
    <ul>
      { list of things }
    </ul>
  </section>
  

  Same markup for the last list items.

  • Lastly, those icons on the last lists, hide them since they are only decoration.

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  Marley Reyna•510

  @MarleyReynaPosted over 3 years ago

  @pikamart Thank you for the feedback! I am pretty sure I got through all of the suggested edits but had a few questions.

  1. What should I specifically set the id of my error message to? As for right now, I have the ID set to 'error-active'.
  2. Is the solution for the theme toggle I used accessible? I made the whole div tab-focusable and allowed it to toggle on the left and right arrow keys. Thanks!

  0

  

  Raymart Pamplona•16,040

  @pikapikamartPosted over 3 years ago

  @MarleyReyna Hey, sorry for the late reply :>>

  1. Yep, I saw the markup right now and you are using the aria-describedBy attribute properly and being linked to the error-message. Also, you don't need the aria-live on the same error-message as well. An aria-live element will only fire if and only if the text-content of the element changes. For example:

  Input has no error:

  <p aria-live=-"polite"> </p>
  

  Input has error, set your input error attributes, the aria-invalid and aria-desscribedBy and the p tag should be like:

  <p aria-live="polite"> github username does not exist </p>
  

  The element will now announce the text-inside it, because at first, the text-content is empty then it is being populated that's why it will fire. Have a look at this simple accessible form that I created you can reference this though you will just have to work a bit more since you are using react.

  2. Nope, still not. As I said, using div is not great for interactive components as they are not informative and does not really used at those. You can see the link that I send from my previous review where the toggle-theme markup of my rest-api challenge is.

  Marked as helpful

  0
• 

  Jimmy Sweeney•350

  @sweenejpPosted over 3 years ago

  Looks good Marley. One critique I have - could you put the infoUnavailable method within your loadUser method? That way you wouldn't have to put infoUnavailable in your JSX. That way you would have all of the data ready to be accessed when you render the component. Probably not a big deal, but I think it would make it a little cleaner.

  Marked as helpful

  0