-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Glib-w2-UsingAPIs #60
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job Glib, good quality work. I left some minor comments as tips and advice for the future.
console.log(data); | ||
const imageElement = document.createElement('img'); | ||
imageElement.src = data.img; | ||
document.body.append(imageElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tip: don't forget to also add an alt for images for accessibility
img.alt = 'image description'
</div> | ||
`; | ||
initEventListeners(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job
renderErrorMessage(`Fetching ${errorContext} failed. ${error.message}`); | ||
return null; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad that you are clearly rendering the error message to the user.
However, it's a bit strange to pass an "errorContext" to this function. Technically it works, but its a strange pattern. We should try to decouple the functions of fetching data from rendering to the UI. You can keep this function as generic as possible without even a try/catch.
You already know the context in whichever function called this genericfetchData
function. You can wrap it in a try/catch in the caller function. If it catches, call the renderErrorMessage there.
|
||
Complete the four functions provided in the starter `index.js` file: | ||
async function fetchAndPopulatePokemons() { | ||
const pokemonListData = await fetchData('https://pokeapi.co/api/v2/pokemon?limit=151', 'pokemon list'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if you wrap this in a try/catch, you already know the context of what is the reason you are calling the generic fetchData
for, so here in the catch block you can then call renderErrorMessage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarification 🙏 As far as I remember the whole idea to add this 'errorContext' parameter was the desire to reduce the amount of code by having less try/catch blocks.
Maybe this pokemon exercise is not a perfect illustration, but what if we have much more functions that use fetchData? I assumed it's good idea to create universal error handler in parent fetch and get rid of all further try/catch blocks ( + describe renderErrorMessage() call only once, instead of doing it in every fetch.
Not sure if it makes sense, but at that moment I thought that I'm actually implementing DRY principle :)
// TODO complete this function | ||
function toggleLoadingAnimation(isSpinnerVisible) { | ||
const spinner = document.querySelector('.spinner'); | ||
spinner.classList.toggle('visible', isSpinnerVisible); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
// Although Promise.race() returns the first resolved promise, it doesn't | ||
// stop other promises that already begun executing. That's why we see other | ||
// dice continue rolling even after Promise.race() resolves. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
'https://api.nobelprize.org/2.0/laureates?birthCountry=Netherlands&format=json&csvLang=en' | ||
); | ||
renderLaureates(laureates); | ||
} catch (err) { | ||
console.error(`Something went wrong: ${err.message}`); | ||
console.log(err.stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason you added this?
Assignment for week2 of UsingAPIs module