GitHub User Search API project

Community feedback

• 

  Vanza Setia•27,715

  @vanzasetia

  Posted almost 3 years ago

  Hi, Trenton Bensheimer! 👋

  Congratulations on finishing this challenge! 👏

  Some suggestions from me.

  • I notice the site has a horizontal scrollbar. I managed to remove the scrollbar by removing the width: 100% on the body element and the width on the .container as well. Setting width: 100% on the body element is unnecessary because by default it is already filling the entire viewport width. For the .container, I suggest only setting a max-width. This way, the container can shrink if ever needed. Also, there's no point in setting the width and max-width with the same value.
  • The indentation of your code is not consistent. It makes it hard for people to understand your code. So, I recommend having a code-formatter like Prettier to handle this automatically for you.
  • For the stats and links, I would recommend making them as ul. It is actually a list with four bullet points.

  That's it! I hope this helps!

  00

  Marked as helpful
• 

  Daniel Brož•40

  @DanoBroz

  Posted almost 3 years ago

  Hi Trenton, I love that you've chosen a mobile-first approach and were able to set this up with VanillaJS. Firstly, as a friendly colleague FE developer, I'd suggest to be more aware of the colours, shadows and paddings provided in the design file, it gives a lot more value and refers how much you care about the project. Secondly, with APIs there's always a chance of some data not being provided, so I'd look into nullability with promises.

  04

  Marked as helpful
• 

  Alan Ibarra•440

  @Ibarra11

  Posted almost 3 years ago

  Im not sure if you notice, but your app overflows in both the X and Y direction. As a result, there is a horizontal scrollbar and a vertical scrollbar. The issue is that the browser adds a default 8px margin to the body. I would suggest to include a CSS reset stylesheet, so you override the default browser styles and your app looks the same across different browsers.

  Another thing I noted is your conditional logic. It's not incorrect ,but I might be more concise to use ternary operators. For Example,

  bio.innerHTML = data.bio ? data.bio : 'this profile has no bio';
  twitter.innerHTML = `<span class="icon"><img src="assets/icon-twitter.svg" alt="twitter icon" /></span> ${data.twitter ? data.twitter : 'Not Available'}`
  

  For your classes, you are adding them though the classList.add but I think you can just do it straight from the HTML. It would be something like this

   const twitterClass = `icon${data.twitter ? "" : " notAvailable"}`;
  twitter.innerHTML = `<span class=${twitterClass}><img src="assets/icon-twitter.svg" alt="twitter icon" /></span> ${data.twitter ? data.twitter : 'Not Available'}`
  

  00

  Marked as helpful
• 

  Babacar Ciss•460

  @Babacar-Ciss

  Posted almost 3 years ago

  hi look very nice but i have some remarks :

  • when user not found it should print "user not found"
  • some layout margin problem in the mobile version

  10
• 

  P

  Oba•130

  @Obatomi01

  Posted over 2 years ago

  Well-done Trenton. You can try checking https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date to make new date objects. You can call methods like getDay(), getMonth() on the prototype created from the Date Class in place of slice method. Good luck

  00