London | 26-ITP-May | Martin Mwaka | Sprint 3 | Programmer humour - #546
London | 26-ITP-May | Martin Mwaka | Sprint 3 | Programmer humour#546Temceo wants to merge 10 commits into
Conversation
Temceo
added
🏕 Priority Mandatory
hey-hammad
added
Review in progress
hey-hammad
left a comment
hey-hammad
left a comment
There was a problem hiding this comment.
Good structure overall, but the solution seems more complex than necessary for a single fetch-on-load scenario.
| const imageElement = document.querySelector("img"); | ||
| const endpoint = `https://xkcd.now.sh/?comic=latest`; | ||
|
|
||
| let imageCache = null; |
There was a problem hiding this comment.
Do we expect fetchImageData() to be called multiple times during the page lifecycle? If not, could we simplify the implementation by removing the cache/promise management and fetching the data directly?
There was a problem hiding this comment.
I have simplified this to collect the data directly
| let imageCache = null; | ||
| let imagePromise = null; | ||
|
|
||
| const state = { |
There was a problem hiding this comment.
Is there a reason we're storing the response in a global state object? Since the data appears to be consumed immediately by renderImage(), could we pass it directly as a parameter instead of maintaining additional state?
There was a problem hiding this comment.
I am now passing the data directly to renderImage
| imgData: {}, | ||
| }; | ||
|
|
||
| async function fetchImageData() { |
There was a problem hiding this comment.
I noticed that function is defined using async, but its using .then() inside. Would it make sense to use await inside fetchImageData() as well?
hey-hammad
added
Reviewed
Temceo
added
Needs Review
2 participants
Self checklist
Changelist
Completed tasks for programmer humour