Github Finder using HTML CSS Vanilla JS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, nice work on this one. Although it is already apparent on the layout that there are stylings missing on this one and the theme toggle is not working at the moment. Also resizing the screen, the layout is not responsive when going into mobile state,

  For some suggestions, here are some:

  • Adding a padding to the top of the body tag so that the layout won't be almost touching the ceiling of the screen.
  • Instead of div, use main tag on the .container selector since you are now wrapping the main-content of the site and also, main tag must be present on a page.
  • I would go h1 for the devfinder website-logo-text.
  • For the theme toggle, I don't know if you already have a reference for this one, but you can have a look at my rest api challenge for the theme toggle. A good markup for such theme toggle should be using radio-buttons inside of a fieldset with a legend.
  • Also, the moon-icon on the theme toggle is only a decorative image. Decorative images 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.
  • It would be really nice to wrap the input or search-bar inside of an form tag so that the markup will be clear and just in general, using form for input that submits data.
  • The search-icon is decorative, hide it for screen-reader using the method above. Same with the 4 icons on the bottom-part of the card, those are all decorative images.
  • Your button should be having a type="submit" on it. Remember that when a button is placed inside a form element, it defaults to type="submit". So imagine if you have a close-button inside the form without specifying type="button" clicking the close-button will submit the form. Be aware of this kind of scenarios.
  • Your input right now currently lacks associated label to it or an aria-label to which will define the purpose of the input element. Always include it so that user will know what they need to give on each input. Make sure that label is pointing to the id of the input as well.
  • Also, another idea to improve accessibility is to have an aria-label announcing whether there is a success result or if there is not. On this one, I saw that when you search for an unknown user, the app still shows information but just undefined. It would be better to just create an error-message that the user is not valid username and announce in the aria-live that user does not exist. This way it will be more straightforward.
  • For each of the user's img, you could use the person's name as the alt value since it makes sense to do so since it will be their profile picture on it.
  • I would go h2 for the username since I suggested h1 on the devfinder logo text:>
  • For those 3 information below the user's info, sine those are "list" of information, using a ul or dl on it would be really nice. On this one, I would go with dl tag:

  <dl>
    <div>
      <dt>Repos</dt>
      <dd>28</dd>
    </div>
  </dl>
  

  You would have 3 div that contains each dt and dd and yes, a div is valid direct child of a dl tag but not for the other list tags.

  • Lastly, just fixing the stylings on the site would be really awesome:>

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  Alejandro•220

  @Alejandro1709Posted almost 3 years ago

  @pikapikamart Thank you so much!! i really appreciate it

  1
• 

  Nam Haϕ820

  @Nam-HaiPosted almost 3 years ago

  It works ! nice one

  I don't understand if you just didn't implemented dark mode or if you did not know how. I'd guess you know how and you just didn't bother ;)

  In case you add no clue, the easy way would just had to change the :root variables of the different color used in the design in the css when the dark mode button is clicked.

  Marked as helpful

  0

  ---

  

  Alejandro•220

  @Alejandro1709Posted almost 3 years ago

  @Nam-Hai Thank you! yea i was struggling for the toggle and persist the state since i was not using react

  0