-
Notifications
You must be signed in to change notification settings - Fork 280
Conversation
profxed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely Done !!
As usual making a very good steps, nice Implementing for classes, I liked your adjustment for the fetchJSON function.
this is not usual, but you have to clean your code a bit when you finish your work.
I left for you a very minor tiny comments, I hope that I didn't miss any thing 🙂.
| xhr.open('GET', url); | ||
| xhr.responseType = 'json'; | ||
| xhr.onreadystatechange = () => { | ||
| if (xhr.readyState === 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can Change this to be:
if (xhr.readyState === xhr.DONE) { // your way same as this, it's included inside the prototype chain.
// 1. would make more sense
// 2. if you have another way than the magic numbers doing
// -- the same thing you should use it otherwise you should
// -- put a little comment telling what the magic number is!!
// the rest of the code...
}| xhr.responseType = 'json'; | ||
| xhr.onreadystatechange = () => { | ||
| if (xhr.readyState === 4) { | ||
| if (xhr.status < 400) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it should be implemented 🥇
|
|
||
| } else { | ||
| descriptionLabel.innerText = "Description :"; | ||
| createAndAppend('td', descriptionLine, { html: repository.description }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have too much creating tr, td tags, For my perspective in this case you should create a function, that may make your life easier.
const createTableRow = (parent=table || null, options={}) => {
const row = createAndAppend('tr', parent);
const label = createAndAppend('td', row, options);
return { row, label }
}Also from line 57 till here you have an innerHTML or 'innerText' multiple times for the same table item, couldn't be like so if you followed my advice for the function:
repository.description ? createTableRow(table, {
html: 'Description :',
class: 'label'
}) : ''Note:
You have to change it until it fit into your code, BUT the same Idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do that but in my code I use the name of the row and td, but I thing you manged that by returning {row, label} I'll try that 👍
| console.log('repo: ', repo); | ||
| const contributors = await repo.fetchContributors(); | ||
|
|
||
| console.log('contributors: ', contributors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log is only for development purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry I forgot to delete it :)
| // Replace this comment with your code | ||
| // | ||
| const table2 = document.getElementById('info-table'); | ||
| if (table2 !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not falsey value which means if a truthy value, that you can do it like so:
if (table2) {/* you code */}
|
|
||
| } else { | ||
| descriptionLabel.innerText = "Description :"; | ||
| Util.createAndAppend('td', descriptionLine, { html: this.repository.description }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these you can Apply the same thing as index.js file
No description provided.