Week3 homework #92
Week3 homework #92
Conversation
|
Hey @end0620, Nice to see your third week homework 🙂, I saw that you did it with classes, I am going to eat something and coming back to review it tonight 😃 |
profxed
left a comment
profxed
left a comment
There was a problem hiding this comment.
Excellent 💯
Hey @end0620, I am so happy that I see your hard work shows up 🙂, you have a very good grasp on javascript 💯.
I left for you some comments for js may help improve your hard work 💪 .
You still don't have any serious issues with your code unless the responsively design 💔 such as:
- best cases in a mobile screen:
- but on 326X522
- same as the previous but in landscape mode.
| @@ -0,0 +1,66 @@ | |||
| 'use strict'; | |||
|
|
|||
| try { | ||
|
|
||
| const repos = await Util.fetchJSON(url); | ||
| repos.sort((repo1, repo2) => { |
|
|
||
| try { | ||
| const repo = this.repos[index]; | ||
| repo.render(document.getElementById('content')); |
There was a problem hiding this comment.
you could call document.getElementById('content') a content for readability.
| } | ||
|
|
||
| render(contributorList) { | ||
| console.log(contributorList); |
| @@ -0,0 +1,66 @@ | |||
| 'use strict'; | |||
|
|
|||
There was a problem hiding this comment.
I see that you have to separate your files yes But you should at least mention that which files are for what !!
As I saw it's same as the one in the assets folder...
| Util.createAndAppend('a', item1, {html: this.data[repoValues[index]], href: this.data.url, target: '_blank'}); | ||
| else if(element === 'Updated') | ||
| { | ||
| let myDate = new Date(this.data[repoValues[index]]); |
There was a problem hiding this comment.
Could updatedDate have a better meaning instead of myDate ??
| Util.createAndAppend('h4', item1, {html: myDate.toLocaleString()}); | ||
| } | ||
| else | ||
| Util.createAndAppend('h4', item1, {html: this.data[repoValues[index]]}); |
There was a problem hiding this comment.
I saw that you are using this.data[repoValues[index]] multiple times, so what you can do is inserting it inside a variable that will declare the idea of it and will make your code much shorter.
| }); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Two empty lines makes the Idea of a new complete section going to be here.
| const repoLabels = ['Repository', 'Description', 'Forks', 'Updated']; | ||
| const repoValues = ['name', 'description', 'forks', 'updated_at']; | ||
| function renderRepo(repo) | ||
| { |
| const contributors = document.getElementById('contribInfo'); | ||
| fetchJSON(repo.contributors_url).then(function(response){ | ||
| response.forEach((contributor, index)=>{ | ||
| if(index >= 10) return; |
There was a problem hiding this comment.
there is no really need to limit the amount of the rendered data on the screen unless you have a hundreds latterly of them.
|
Oh I forgot you should be aware of your files Directory names, and you should just separate your different kind of files files. |
3 participants
No description provided.