London | 26-ITP-May | Tomislav Dukez | Sprint 3 | Programmer Humour - #537
London | 26-ITP-May | Tomislav Dukez | Sprint 3 | Programmer Humour #537tomdu3 wants to merge 7 commits into
Conversation
tomdu3
added
🏕 Priority Mandatory
tomdu3
added
Needs Review
hey-hammad
left a comment
hey-hammad
left a comment
There was a problem hiding this comment.
Nice work overall. I noticed a few code quality issues that would be good to clean up. Thanks
| @@ -0,0 +1,26 @@ | |||
| const imgEl = document.getElementsByTagName("img")[0]; | |||
There was a problem hiding this comment.
is there a better way of getting the specific image element without using the tag?
Think about the following:
Right now we're assuming there is always 1 image element in the html, what if that changes to 0?
There was a problem hiding this comment.
I solved the problem with js element creation and fallbackImg integration which solves both problems
| } | ||
| }; | ||
|
|
||
| let data; |
There was a problem hiding this comment.
Is the outer let data; still needed here?
There was a problem hiding this comment.
removed data variable initialisation
| }; | ||
|
|
||
| let data; | ||
| fetchData().then((data) => { |
There was a problem hiding this comment.
Would it make sense to keep the async flow consistent by using async/await when consuming fetchData() as well, rather than mixing async/await and .then()?
Also, should we add a guard for the image element before setting imgEl.src?
There was a problem hiding this comment.
fixed fetch logic and guard for image
hey-hammad
added
Review in progress
tomdu3
added
Needs Review
hey-hammad
added
Review in progress
2 participants
Learners, PR Template
Self checklist
Changelist
Added Programmer Humour Task files
index.html,style.css, andscripts.jsassets/no_image.webp, while image is being fetched, or if API call fails