Skip to content
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

Lidya-w2-usingAPIs #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lidyaT21
Copy link

@lidyaT21 lidyaT21 commented Feb 4, 2025

No description provided.

@lidyaT21 lidyaT21 closed this Feb 4, 2025
@lidyaT21 lidyaT21 reopened this Feb 4, 2025
@abbathaw abbathaw self-assigned this Feb 16, 2025
Copy link

@abbathaw abbathaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Lidya. I left you some comments and tips on code improvements but they are not a blocker for me. I would have requested changes for ex2 to be fixed before an approval usually if this was a real production code used by real users ;) Best of luck on the next modules.

return data;
} catch (error) {
console.error('Error fetching data:', error.message);
displayError(error.message);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job on displaying an error message to the user.

I didn't run this code locally but I can see that you might end up displaying the message twice to the user. Because I see you are throwing the error and in lines 56-58 you are catching it again and calling displayError again.

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. And since you already have a try/catch in the functions calling this e.g. fetchAndPopulatePokemons and fetchImage they will handle catching any errors and handing the UI display.

Great job though.

}

// ! Do not change or remove the code below
if (process.env.NODE_ENV !== 'test') {
main();
}

// TODO Replace this comment by your explanation that was asked for in the assignment description.
/* When Promise.race() resolves, the first die to finish rolling determines the final result, the remaining dice continue rolling in the background because JavaScript promises
cannot be cancelled once started. They will eventually resolve, but their results are ignored.*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants