Week2 #76
Week2 #76
Conversation
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
Hi Marwan, there are some issues with HTML you generate that I would like you to fix. You also have a bit more CSS styling work to do. And please try to make your app responsive so that it looks good on mobile too.
| @@ -0,0 +1,73 @@ | |||
|
|
|||
There was a problem hiding this comment.
Why did you copy this file into the Week1/assets folder?
| @@ -0,0 +1,23 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Why did you copy this file into the Week1/assets folder?
| @@ -0,0 +1,47 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Why did you copy this file into the Week1/assets folder?
homework/src/index.js
Outdated
| const header = createAndAppend('h1', div, { html: "HYF Repositories ", class: 'header' }); | ||
| const select = createAndAppend('select', header, { class: "chose" }); | ||
| const table = createAndAppend('table', div, { class: 'font' }); | ||
| const tablebody = createAndAppend('tablebody', table); |
There was a problem hiding this comment.
There is no HTML element with the tag name tablebody. Did you mean tbody?
| const select = createAndAppend('select', header, { class: "chose" }); | ||
| const table = createAndAppend('table', div, { class: 'font' }); | ||
| const tablebody = createAndAppend('tablebody', table); | ||
| const title = createAndAppend('a', tablebody); |
There was a problem hiding this comment.
A tbody element may only contain tr children.
| select.addEventListener('change', (event) => { | ||
| const index = event.target.value; | ||
| const repository = repositories[index]; | ||
| title.innerHTML = "Repository: " + repository.name; |
There was a problem hiding this comment.
The title variable is reference to an a tag. You are setting the innerHTML of that tag, but not the href attribute. Therefore it does not work as a link.
| title.innerHTML = "Repository: " + repository.name; | ||
| title2.innerHTML = "Forks: " + repository.forks; | ||
| descripTitle.innerHTML = "Description: " + repository.description; | ||
| newTitle.innerHTML = "Updated: " + new Date(repository.updated_at).toLocaleString(); |
There was a problem hiding this comment.
title2, descripTitle and newTitle (can you think of better names?) refer to tr elements. The only allowed content for tr elements are td and th elements. They cannot contain text or anything else. Therefore, first create a td element and append it to the tr. Then do whatever you like inside the td element.
There was a problem hiding this comment.
You still did not address this issue: a tr element may only contain td and th elements. Nothing else, not even text.
| }); | ||
|
|
||
|
|
||
| select.addEventListener('change', (event) => { |
There was a problem hiding this comment.
We are not seeing information about the first selected repository when the page is first loaded. Only when we make a change, then we see the information. Can you make it so that the initial repo info is also displayed?
homework/src/index.js
Outdated
|
|
||
| }); | ||
| }) | ||
| .catch((err) => console.log(err)); |
There was a problem hiding this comment.
When there is an error fetching the contributors we can only see that error in the console which regular users do not have open. Can you render it to the HTML page instead?
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
It looks I forgot to finish my review. There were some changes I was planning to request. Can you implement them in your latest week 3 PR?
| title.innerHTML = "Repository: " + repository.name; | ||
| title2.innerHTML = "Forks: " + repository.forks; | ||
| descripTitle.innerHTML = "Description: " + repository.description; | ||
| newTitle.innerHTML = "Updated: " + new Date(repository.updated_at).toLocaleString(); |
There was a problem hiding this comment.
You still did not address this issue: a tr element may only contain td and th elements. Nothing else, not even text.
| contributors.forEach(contributor => { | ||
| const avatar = createAndAppend('img', container, { class: 'avatar' }); | ||
| avatar.setAttribute("src", contributor.avatar_url); | ||
| const list = createAndAppend('ul', container, { class: 'list' }); |
There was a problem hiding this comment.
You should a single list (one ul only) of contributors. Here you are creating a new list for each contributor.
| @@ -1,3 +1,99 @@ | |||
| .alert-error { | |||
There was a problem hiding this comment.
Your app is not responsive. This is what it looks like on a iPhone 6:
| title.innerHTML = "Repository: " + repository.name; | ||
| titleFork.innerHTML = "Forks: " + repository.forks; | ||
| descripTitle.innerHTML = "Description: " + repository.description; | ||
| newTitle.innerHTML = "Updated: " + new Date(repository.updated_at).toLocaleString(); |
There was a problem hiding this comment.
My comment from last homework is still not addressed: titleFork, descripTitle and newTitle refer to tr elements. A tr element may only contain td or th elements, nothing else, not even text.
3 participants
No description provided.