GitHub User Search App | HTML CSS Sass JavaScript (Async Await)

Community feedback

• 

  T

  Grace•31,830

  @grace-snowPosted almost 2 years ago

  Nice enhancements! You don't need the aria-describedby on the theme switcher inputs though. They are part of a labelled fieldset already so this is just duplicating and making the legend repeat

  Marked as helpful

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 2 years ago

  @grace-snow

  Hi, Grace! Thank you for the feedback.

  I removed the aria-describedby from the radio inputs.

  By the way, I got the inspiration for the theme switcher from your website, FED Mentor .

  Home - FED Mentor

  I notice that the radio inputs have aria-describedby that is pointing the <legend>. Also, there is role="radiogroup" on the <fieldset> element.

  I followed the HTML markup and the aria-describedby. 😅

  0
• 

  T

  Grace•31,830

  @grace-snowPosted over 2 years ago

  Other questions...

  • I don't think you need hidden headings on this
  • role switch seems to be working well. I've not done any thorough testing or reading up about role switch before, but seems to be working well on voiceOver thanks to you adding that sr label that makes it all make sense
  • treatment of profiles with no info seems very sensible overall. I definitely would not have 'Not available' for a heading though. Instead do something like "janedoe Github profile" or similar
  • color theme management is great
  • I wouldn't worry much about names. As long as you are consistent in approach and they make sense that's fine

  Marked as helpful

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @grace-snow

  Hi Grace! 👋

  For the role="switch", I get the inspiration from Twitter. Jen Simmons created a tweet where she is explaining how to create a theme switcher with only HTML and CSS (using :has()). Then, there is someone called Mike-麥-Mai/index.html, he is telling Jen Simmons to add role="switch" to the checkbox element.

  For the heading, I have just fixed it. Now, if the user has no name, then the heading will have text content like "janedoe's GitHub Profile".

  Thank you for answering my questions! 👍

  P.S. I would improve the project step-by-step instead of just trying to fix or improve the project at once. I am afraid that fixing all things at once will make me do a lot of git restore . (or git reset --hard). So, I will reply to each feedback once I have fixed the issue or when I have any questions. I hope that you are okay with this. 🙂

  1
• 

  T

  Grace•31,830

  @grace-snowPosted over 2 years ago

  Re the search input question: https://adrianroselli.com/2019/07/ignore-typesearch.html

  I did have a little trouble using the input on my phone, but rarely use voiceover on there so it was probably my own fault! I found it strange not to have the search keyboard on there though... Adding inputmode="search" may help with that.

  Marked as helpful

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @grace-snow

  I have made the changes regarding the search input. Now, the input is using type="text" with inputmode="search" attribute. Also, I removed the div with role="search" element and I didn't add role="search" to the form element.

  Also, thank you for the article! 👍

  0
• 

  T

  Grace•31,830

  @grace-snowPosted over 2 years ago

  Overall, accessibility on this is excellent, well done. There's just a few things I think need tweaking :)

  1. I'm not sure why you've aria-hidden the 'search GitHub username' text then added a hidden label with aria-labelledby on the input. If there is visible text on the screen that can be a label, it's always much better to use that text. Speech tech users would say 'Enter search Github username' and it would activate the input using that label rather than them trying to use a hidden label which may use different wording

  2. There needs to be some kind of feedback to assistive tech users that their search has been successful. Either that can be done by adding an aria-live attribute to the results list or adding a hidden aria-live region that says something like 'results updated to user X'

  Other than that

  • I'd consider using a description list for the bottom section (location, website etc) as that is designed for key-value pairs. Not essential though
  • I'd use a different technique for that section's layout too, to prevent overflow when items are 2 long to fit in a 2 column layout
  • consider making external links open in a new window. If so, make sure you include some warning text to that effect (e.g. a title attribute)
  • I don't think you need as many media queries (you may not need any in fact). For example, doing body padding in % or using min() or clamp() etc would remove the need for it
  • on the document title, I'd put dev-finder before the profile name. The site is more important. And - it's an extra not really included in the challenge but would be nice - maybe make a new favicon for the site. Even just the dark blue background with a 'd' in the middle would be better than using the frontend mentor one I think

  Well done again on this, really nice work!

  Marked as helpful

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @grace-snow

  Thank you for telling me that the website has excellent accessibility! It motivates me to be better at web accessibility! 🙌

  Regarding the way I label the input, as far as I know, it's best to have the label before the input element. So, I set up a hidden label to label the input. After that, the visible label element is used as the floating label (for visual users). Then, when the user fills the search input, the placeholder moves above the search input instead of going away (like a normal placeholder). What do you think about this?

  I added a hidden live region. Here is the HTML markup.

  <p class="sr-only" 
     role="status"
     aria-live="polite"
     aria-atomic="true"></p>
  

  Then when the search is successful. The text will get injected.

  <p class="sr-only" 
     role="status"
     aria-live="polite"
     aria-atomic="true">
    results updated to janedoe's GitHub profile
  </p>
  

  I tested it. The result is, after the search is successful, both TalkBack and Narrator pronounce it the same way, "results updated to janedoe's GitHub profile".

  Regarding the description list (dl), I tried to use it and it turned out that it causes HTML issues. (I ran the HTML code through validator)

  The HTML markup is the following.

  <dl class="result__details" role="list">
    <div class="result__item js-location">
      <svg
        class="result__icon"
        width="14"
        height="20"
        aria-hidden="true"
        focusable="false"
      >
        <use href="/svg/sprite.svg#location"></use>
      </svg>
      <dt class="sr-only">Location:</dt>
      <dd class="result__location js-value">San Francisco</dd>
    </div>
  </dl>
  

  The error:

  Error: Element svg not allowed as child of element div in this context. (Suppressing further errors from this subtree.)
  
  From line 230, column 15; to line 236, column 15
  
            <svg↩                class="result__icon"↩                width="14"↩                height="20"↩                aria-hidden="true"↩                focusable="false"↩              >↩     
  
  Content model for element div:
    - If the element is a child of a dl element: one or more dt elements followed by one or more dd elements, optionally intermixed with script-supporting elements.
    - If the element is not a child of a dl element: flow content.
  

  So, the site is still using ul element.

  For external links, I don't make those links open in a new window because I simply don't see any benefits to doing it. 😅

  (Based on my research, there are some situations where it is preferable from an accessibility perspective to open a new window or tab.)

  For the title, I do it intentionally because I notice the pattern in the "Writing for Web Accessibility" article that the site name is always at the end (e.g. Latest News • Space Teddy Inc., Buy Your Bear (Step 1 of 3) • Space Teddy Inc.).

  For the styling, I would try improving the responsiveness of the site using slinky (e.g. clamp, CSS Grid, etc). Currently, I haven't done anything for the styling. But, hopefully, in the near future, the site is much more responsive with fewer media queries (or even without media queries).

  About the favicon, I changed the favicon to the search icon with dark blue background. It's a nice little touch for the site! 🙌

  Once again, thank you so much for the feedback. I couldn’t be at this point without your guidance. I can’t thank you enough!

  0

  

  T

  Grace•31,830

  @grace-snowPosted over 2 years ago

  @vanzasetia yeah you would need to use role presentation on anything that isn’t allowed as a child of an element

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @grace-snow I have tried adding role="presentation" to the svg and it is still producing the same error.

  0
• 

  Yazdun•1,310

  @YazdunPosted over 2 years ago

  Hi Vanza, Great job on the challenge, I specially love the fact that you added a query in the url for the searched user, that's a really nice touch.

  Here is quick issue that Matt Seidel noticed on my solution and then I could fix it. Try searching mseidel819 in your app and you will see a weird overflow which doesn't look nice, you may wanna take care of that.

  3

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @Yazdun

  Hi Yazdun! Thank you for highlighting the issue! Also, It's good to see your comment on this solution as well! 😀

  I have just fixed the overflowing text issue. So, if you take a look at Matt Seidel's GitHub profile on my website, it is no longer overflowing. 😁

  Speaking of which, I find a GitHub user who has the longest text content for each data. I think this user will cause a lot of overflowing issues for people who are doing this challenge (including me). 😆

  2
• 

  T

  Grace•31,830

  @grace-snowPosted over 2 years ago

  I don't think the search role is necessary at all. You already have a clearly labelled input. That article you referenced is 6 years old and I generally steer away from anything that tells me to put a role on a div. It just isn't necessary. Don't over-complicate things

  1
• 

  Enmanuel Otero Montano•2,155

  @Enmanuel-Otero-MontanoPosted over 2 years ago

  Hello Vanza!

  Excellent. I went to see the code to see if you implemented the catch ( to handle the error ) since it is your first challenge consuming API and of course you implemented it 😂👌.

  On the other hand, I would like to read the article that says that an input type="text" is better than an input type="search", but it seems to me that implementing the input type="search" is a help in validation , so without reading the article I vote for the input type="search".

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @Enmanuel-Otero-Montano

  Hi! Thank you for the comment! 👍

  Yeah, I also think search input would be a suitable choice (since it is a search form). That is why I chose to use the search input instead of the text input when I was building the project. Good to know you have the same opinion! 😀

  0

  

  Enmanuel Otero Montano•2,155

  @Enmanuel-Otero-MontanoPosted over 2 years ago

  @vanzasetia Hi!

  After I made the comment I kept thinking about what I said about input type="search" and validation and it doesn't really help validation, when I said that I was thinking about **input type="url"**🤦‍♂️😂, but what if the input type="search" helps in the user experience, since when the site is viewed from a cell phone the keyboard changes depending on the type of input that has the focus. So I'm still upvoting the input type="search" in this case.

  Have a nice day/night Vanza!

  1

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @Enmanuel-Otero-Montano

  Haha! You are right! It doesn't have any validation. But, as you said, it helps the mobile users to get the right keyboard layout which in this case they will get the "Search" button (like this website has shown, http://mobileinputtypes.com/).

  Thank you! Have a nice weekend! 😄

  0

  

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  😁 thanks for always making helpful comments @Enmanuel-Otero-Montano and @vanzasetia reading through those comments helps me improve 😘. Concerning that I think one can also use the "inputmode" attribute of the input. Like <input type="text" inputmode="search"> it works for textarea too. There are also other values for the input mode.

  I'm not saying input type search is a bad idea though

  This 1 minute video explains it well.

  1

  

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  @vanzasetia lest I forget, I have sent you a connect request on LinkedIn l, kindly accept it when you're online

  0

  

  Enmanuel Otero Montano•2,155

  @Enmanuel-Otero-MontanoPosted over 2 years ago

  @EngineerHamziey Hello!

  I am glad to know that they help you to improve our comments. Regarding the video, it is information that I did not know, but I cannot think of a situation in which it could help me, since there are the native html elements that are adjusted according to the situation. I also believe that the simpler, the better.

  Anyway, thank you for the information.

  Cheers!

  1

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @EngineerHamziey Hi Hamzat! 👋

  You are welcome! I am glad to know that my comments help you improve your front-end skill! 😄

  Thank you for telling me about inputmode attribute! I never heard about this attribute before.

  I did quick research about inputmode and found that it is used to give hints about the type of data that might be entered by the user. This can helps mobile users to get the right virtual keyboard layout (the same benefit as using type attribute).

  But, unlike input with type attribute, it doesn't have any validations. So, for example, if I have <input inputmode="email"> then it doesn't have any native validation unlike <input type="email">.

  In this case, I think your suggestion is fine. But, I would stick to what I already did (which is by using <input type="search">) because the users require to input a search query. ("Inputs that require a search query should typically use <input type="search"> instead.", from MDN documentation).

  Lastly, about the LinkedIn invitation, I checked the "Invitation" and I didn't see your invitation (my invitation was empty). So, you may want to try to create a new invitation. 🙂

  2