Js3 Week3 Barbaros #84
Conversation
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
Your error handling does not work. For my other feedback, please refer to my week 2 comments.
homework/src/index.js
Outdated
| error => { | ||
| createAndAppend('div', root, { html: error.message, class: 'alert-error', role: "alert" }); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Your error handler doesn't work. Have you tried it?
There was a problem hiding this comment.
I corrected code. I had not noticed this error before i uploaded it
|
Dear @remarcmij , I have made changes by the help of your comments. Would you please reexamine my assignment ? Thanks for your efforts. |
homework/src/index.js
Outdated
| } | ||
| }); | ||
| xhr.onerror = () => { | ||
| reject(alert("Opppsss, Network Error")); |
There was a problem hiding this comment.
The reject method must be called with error information that is to be passed to .catch() handlers. Here you are passing a call to alert(). The alert() function returns undefined. So the value that is passed to the .catch() is undefined. This causes the catch() handler to produce a runtime error in line 72, because it is expecting an Error object with a message property.
index.js:72 Uncaught (in promise) TypeError: Cannot read property 'message' of undefined
Moreover, you should not do any DOM manipulation or anything to do with the UI in this fetchJSON() function. This is a violation of the important principle of Separation of Concerns. The fetchJSON() should be concerned with fetching data only. In line 12 it is done correctly.
For instance, see this Stack Overflow question:
https://stackoverflow.com/questions/98734/what-is-separation-of-concerns
There was a problem hiding this comment.
yes, i think it is corrected
There was a problem hiding this comment.
@remarcmij Sorry for my errors. I think i understood what you mean and tried to correct it. Now i can not see an error. I hope it is done now. If you think it needs more corrections, please warn me again. Thanks
homework/src/index.js
Outdated
| } | ||
| }); | ||
| const HYF_REPOS_URL = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100'; | ||
| const root = document.getElementById('root'); |
There was a problem hiding this comment.
You should place this statement definition root inside function main() so that it is guaranteed to be executed through window.onload. The root div may not have been created by the browser yet when this statement is executed as this point. The 'load' event is guaranteed to executed when the web page and all its resources are completely loaded.
There was a problem hiding this comment.
It is inside the main function now
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
All is well now. Thanks for following-up on the feedback.
|
i have seen that in 3 places root seems undefined. I have corrected them. I hope it is more better now. thanks again |
|
i tried to add root as extra parameter to render functions in order to cancel undefined error. |
3 participants
No description provided.