Conversation
mattiaslundberg
left a comment
There was a problem hiding this comment.
I know that it's a lot of comments. Most are for general improvement of the code and so that you can learn. The only one I want to see fixed before approving is that you should use the main function. (Please read the rest of the comments and have them in mind for your future homeworks)
| const HYF_REPOS_URL = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100'; | ||
|
|
||
| function displayContrib(contributor, contributions, avatar) { | ||
| const eachPersonUl = document.createElement('ul'); |
There was a problem hiding this comment.
I find this function and the displayRepo function hard to read, try to add some blank lines where it's possible to logically separate different things. For example:
const eachPersonUl = document.createElement('ul');
const contributorName = document.createElement('li');
contributorName.innerText = `Username: ${contributor}`;
eachPersonUl.appendChild(contributorName);
const eachContributions = document.createElement('li');
eachContributions.innerText = `Contributions: ${contributions}`;There was a problem hiding this comment.
Also, as the code provides the createAndAppend function, it's a good idea to actually use it. That would help with readability.
| eachPersonUl.appendChild(eachContributions); | ||
| contribDiv.appendChild(eachPersonUl); | ||
| const eachAvatar = document.createElement('IMG'); | ||
| eachAvatar.innerhtml = `<img src=${avatar}'>`; |
There was a problem hiding this comment.
I find this row a bit strange (and it looks like it isn't working as expected). You can actually set the attribute like this (and I think it would work):
eachAvatar.setAttribute('src', avatar);|
|
||
| const selectElement = document.querySelector('.selector'); | ||
|
|
||
| selectElement.addEventListener('change', event => { |
There was a problem hiding this comment.
It looks like you're doing this outside of the main function. This can cause problems down the line (but looks like it's working during my tests), as some things might not be ready before window.onload is called. Please give moving the code inside the main function a try!
| } else { | ||
| createAndAppend('pre', root, { text: JSON.stringify(data, null, 2) }); | ||
| } | ||
| const repoSelector = document.querySelector('#repoSelect'); |
There was a problem hiding this comment.
From what I can see these variables aren't used, it's always a good idea to remove stuff that you don't need. Keeping them around will likely confuse you the next time you look at the code.
| let repoURL = `https://api.github.com/repos/HackYourFuture/${repo}`; | ||
| let contribURL = `https://api.github.com/repos/HackYourFuture/${repo}/contributors`; | ||
|
|
||
| main(HYF_REPOS_URL); |
There was a problem hiding this comment.
I'm not sure why you're calling main here, you should probably remove it.
| .map(repo => repo.name) | ||
| .sort() | ||
| .forEach(name => { | ||
| createAndAppend('OPTION', repoSelector, { text: name, value: name }); |
There was a problem hiding this comment.
It seems like you add options to the select box multiple times, see if you can figure out what's causing that!
| padding: 10px; | ||
| } | ||
|
|
||
| #footer { |
There was a problem hiding this comment.
This is never used, so it's best to remove it.
| color: #444; | ||
| } | ||
|
|
||
| #repo { |
There was a problem hiding this comment.
The contents of #repo and #contribDiv looks the same, is there some way to avoid repeating all the code?
| padding: 5px; | ||
| } | ||
|
|
||
| #main-content { |
There was a problem hiding this comment.
This isn't used, so it's best to remove it.
| margin: 20px; | ||
| } | ||
|
|
||
| @media screen and (max-width: 600px) { |
No description provided.