NW6| BakhatBegum | Module-JS3 | Feature/humor |Sprint-3 #300
Conversation
|
BakhatBegum
changed the title
There was a problem hiding this comment.
Great work, Bakhat, your app works wonderfully! 🚀
I've left a few comments for you to check out, reach out on Slack if you have any questions.
Also be careful of your branches, to make sure that other exercises aren't accidentally included in the PR (I can see the book-library/ directory has come in to this PR).
| } | ||
| }); | ||
| } | ||
| getImageFetch(); |
There was a problem hiding this comment.
Because we call getImageFetch() from inside the displayData() function, we don't need to call it here 🙂
| } | ||
| const image = data.img; | ||
|
|
||
| const createImage = document.createElement('img'); |
There was a problem hiding this comment.
I really like how you create the image element inside JavaScript! It means the user doesn't see a 'broken image' when they first load the page, creating a nice user experience
| if(!data){ | ||
| console.log("data can not found"); | ||
| } | ||
| const image = data.img; |
There was a problem hiding this comment.
Could this variable name be a bit more descriptive? For example, imageUrl would make it clear that this is the link to the image
| } | ||
| displayData(); | ||
|
|
||
| window.onload = displayData; |
There was a problem hiding this comment.
When I launch the page, I see the same image appear twice. Having a look at these final lines of the JavaScript file, can you think why that might be?
| function getImageFetch(){ | ||
| return fetch('https://xkcd.now.sh/?comic=latest').then((response) => { | ||
| if(!response.ok){ | ||
| return "Error"; |
There was a problem hiding this comment.
Instead of just returning the text "Error", we could instead throw an exception. This is a large topic to cover here, so reach out on Slack if you have any questions on what this means!
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.