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

Sertan-w2-UsingAPIs #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sertanrdn
Copy link

No description provided.

@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.

Awesome job Sertan. I left you some comments and tips on code improvements but they are not a blocker for me to approve this assignment review.

Comment on lines +21 to +30
.then((response) => {
if (response.ok) {
return response.json(); // If status is ok convert the response to JSON file
} else {
throw new Error(`HTTP Error! status: ${response.status}`); // If it is not throw error
}
})
.catch((error) => {
throw new Error(`Network error: ${error.message}`); // Catch the possible errors
});

Choose a reason for hiding this comment

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

this is good, but since you already learnt it you can just use async/await.

async function requestData(url) {
  const res = await fetch(url);
  if (!res.ok) {
    throw new Error(`HTTP ${res.stat} ${res.statusText}`);
  }
  return res.json();
}

fetchImage(selectedUrl);
});
} catch (error) {
console.error('Error populating pokemon list:', error);

Choose a reason for hiding this comment

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

this is beyond the assignment but in real applications make sure you display a better error to the user otherwise they will never know why the app is not working as expected, in your case the select element will be disabled and nothing showing up. But if you had an error message, I can at least know something went wrong.

}

// ! 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.
// Promise.race() waits the first promise in the array to resolve/reject. It will not affect the other promises so they can continue until they resolve/reject.

Choose a reason for hiding this comment

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

👌

death.place &&
death.place.locationString &&
death.place.locationString.en
) {

Choose a reason for hiding this comment

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

Nice job on checking that the full object is there. However, if a deep object such as location string in .en is undefined, you might not show the full thing even if there was a death object. So keep in mind sometimes when you build apps in the future that if your condition is too strict.

one way to make the outer condition shorter using optional chaining (?.)

if (death?.date && death?.place?.locationString?.en) {

but you can also do this with both optional chaining (?.) and the nullish coalescing operator (??)

if (death) {
  console.log(`Death: ${death.date ?? 'Unknown Date'}, ${death.place?.locationString?.en ?? 'Unknown Location'}`);
} else {
  console.log('Still alive');
}

@@ -20,10 +45,11 @@ function renderLaureates(laureates) {

async function fetchAndRender() {
try {
const laureates = getData(
const data = await getData(

Choose a reason for hiding this comment

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

this also works

const { laureates } = await getData(

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