Apis week2 #5
Apis week2 #5
Conversation
There was a problem hiding this comment.
Hi @Veronika121, some pretty good work. But there are some issues with the pokemon exercise that I would like you to have another look at.
| return response.json(); | ||
| } | ||
| throw new Error('Request failed'); | ||
| }); |
There was a problem hiding this comment.
Some developers maintain a strict rule: either all .then() chains across your code or async/await. I'm not part of that school. For a function like this I might also choose to use .then() while using async/await elsewhere.
| const body = document.querySelector('body'); | ||
| const h1 = document.createElement('h1'); | ||
| h1.textContent = error; | ||
| body.appendChild(h1); |
There was a problem hiding this comment.
This works fine, but you also use this and remove line 39.
| body.appendChild(h1); | |
| document.body.appendChild(h1); |
| renderImage(data); | ||
| } catch (error) { | ||
| renderError(error); | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('load', main); |
| } | ||
|
|
||
| function fetchAndPopulatePokemons(/* TODO parameter(s) go here */) { | ||
| // TODO complete this function | ||
| function fetchAndPopulatePokemons(data) { |
There was a problem hiding this comment.
As the name implies, this function is supposed to fetch the pokemons as a result of pressing the button labelled Get pokemons'. Instead you are fetching them in main()` at application start-up.
| fetchImage(event.target, listOfPokemons) | ||
| ); | ||
|
|
||
| buttonGetPokemon.removeEventListener('click', showListOfPokemons); |
There was a problem hiding this comment.
On the one hand, is it good that you prevent the pokemons to be added again if the button is pressed more than once. But removing the event handler does not give visual clue to the user that the button is no longer functioning. It would be better to disable the button rather than removing the event listener. That will then grey out the button, giving the user the desired visual clue.
|
|
||
| /*Some dices continue rolling after the promise returned by `Promise.race()` resolves | ||
| because `Promise.race()` resolves when the first of the promises in the array is resolved | ||
| but `Promise.race()` doesn't disturbs to other promises to be resolved. |
| console.log(`Death: ${death.date}, ${death.place.locationString}`); | ||
| console.log(`Birth: ${birth.date}, ${birth.place.locationString.en}`); | ||
| if (death) { | ||
| console.log(`Death: ${death.date}, ${death.place.locationString.en}`); |
| @@ -21,7 +23,7 @@ function renderLaureates(laureates) { | |||
|
|
|||
| async function fetchAndRender() { | |||
| try { | |||
| const laureates = getData( | |||
| const { laureates } = await getData( | |||
| 'http://api.nobelprize.org/2.0/laureates?birthCountry=Netherlands&format=json&csvLang=en' | |||
| 'Death', | ||
| `${death.date}, ${death.place.locationString.en}` | ||
| ); | ||
| } |
| const ul = createAndAppend('ul', document.body); | ||
| laureates.forEach((laureate) => renderLaureate(ul, laureate)); | ||
| } | ||
|
|
||
| async function fetchAndRender() { | ||
| try { | ||
| const laureates = getData( | ||
| const { laureates } = await getData( | ||
| 'http://api.nobelprize.org/2.0/laureates?birthCountry=Netherlands&format=json&csvLang=en' |
|
@remarcmij Thank you for your feedback! I've implemented it. |
There was a problem hiding this comment.
Hi @Veronika121, thanks for the updates. I will now approve your PR.
No description provided.