Conversation
harditsingh
left a comment
There was a problem hiding this comment.
Great job Mudar, your app looks very good and works well! I have some comments though which you should improve in the next assignment, please read the comments below.
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <meta http-equiv="X-UA-Compatible" content="ie=edge"> | ||
| <link rel="stylesheet" href="Style.css" /> |
There was a problem hiding this comment.
Somehow your CSS file was not linked, turns out that stylesheet is 'style.css' and not 'Style.css' (yep, the value for the source is case-sensitive)
|
|
||
| function callback(error, data) { | ||
| if (error !== null) { | ||
| } else { |
There was a problem hiding this comment.
You didn't handle the error! If the server is unreachable, the page doesn't load at all. This isn't the best thing to do as the user would be wondering what happened. You should put text on the page saying that there was an error retrieving the data from the server.
| repo.forEach(repo => { | ||
| const option = createAndAppend('option', select); | ||
| option.innerHTML = repo.name; | ||
| option.setAttribute('value', repo.name); |
There was a problem hiding this comment.
You forgot to sort the elements of the list in an alphabetical order!
|
|
||
| function repoInfo(repoName) { | ||
| const urlContributor = 'https://api.github.com/repos/HackYourFuture/' + repoName + '/contributors'; | ||
| const repoUrl = 'https://api.github.com/repos/HackYourFuture/' + repoName; |
There was a problem hiding this comment.
You should not put static links inside functions, you should save these strings in a variable and then get the value from the variable. The way you did it gets the work done, but it will cause problems when you try to change the link (and forget to change it on some places).
| repoName.innerHTML = 'repo: ' + repoInfo.name; | ||
| forks.innerHTML = 'Forks: ' + repoInfo.forks_count; | ||
| updated.innerHTML = 'Updated: ' + repoInfo.updated_at; | ||
| p.innerHTML = 'Description: ' + repoInfo.description; |
There was a problem hiding this comment.
You shouldn't use spaces, tabs, etc. to format the text on the webpage, always use CSS for this. (Can you please correct this in the next assignment when you get time?)
Hi,
Please review my work.
Thanks - Mudar