Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Conversation

@marwanaboudaoud
Copy link

No description provided.

Copy link

@profxed profxed left a comment

Choose a reason for hiding this comment

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

Hi @marwanaboudaoud, I can't say anything until I see some changes you make, because the review at the end not for us it's only for you and make you better with what you are doing.

I see your hard work And you may be so trust in your code but I don't see any progress if you didn't submit it by improve that thing is wrong for me...

Overall you have a very nice homework but you should follow our comments! 🙂

reject(new Error(`Network error: ${xhr.status} - ${xhr.statusText}`));
}
};
xhr.onerror = () => cb(new Error('Network request failed'));
Copy link

Choose a reason for hiding this comment

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

what is cb ?

const select = createAndAppend('select', header, { class: "chose" });
const table = createAndAppend('table', div, { class: 'font' });
const tablebody = createAndAppend('tbody', table);
const title = createAndAppend('a', tablebody);
Copy link

Choose a reason for hiding this comment

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

this line has two comments in a previous homeworks, it's nice to see some progress...

});


select.addEventListener('change', (event) => {
Copy link

Choose a reason for hiding this comment

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

same as week2 comment

title.innerHTML = "Repository: " + repository.name;
titleFork.innerHTML = "Forks: " + repository.forks;
descripTitle.innerHTML = "Description: " + repository.description;
newTitle.innerHTML = "Updated: " + new Date(repository.updated_at).toLocaleString();
Copy link

Choose a reason for hiding this comment

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

});
}

catch (error) {
Copy link

Choose a reason for hiding this comment

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

Nice using of catch ❤️

contributors.forEach(contributor => {
const avatar = createAndAppend('img', container, { class: 'avatar' });
avatar.setAttribute("src", contributor.avatar_url);
const list = createAndAppend('ul', container, { class: 'list' });
Copy link

Choose a reason for hiding this comment

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

Same as week2 comment

color: red;
}

* {
Copy link

Choose a reason for hiding this comment

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

Same as week2 comment

@marwanaboudaoud
Copy link
Author

hello @JawharBairakdar
i just want to say that i resolve many problems here can you see my homework again and let me know if there is any thing i have to do it
thanks alot :)

Copy link

@profxed profxed left a comment

Choose a reason for hiding this comment

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

Hey @marwanaboudaoud I see that you changed some of this homework, and reviewed as requested.

So the changes has been made here so far are:

  • some CSS styling
  • removing the cb from line 17 which is not the solution for handling errors in a Promise function.

What we still have:

  • still need some changes from the CSS part.
  • the HTML tag order.
  • the Initial rendering.

How ever I am Not going to ask for any further changes and I will just approve it because As I said it's for you at the end 🙂

}
};
xhr.onerror = () => cb(new Error('Network request failed'));
xhr.onerror = () => (new Error('Network request failed'));
Copy link

Choose a reason for hiding this comment

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

You are not doing any thing with the error in this case.

what you could do is to reject the error to make it available for the catch method, with that you can handle your error to show the user any information accords and happening.

  • if it's an error so you did reject it.
  • if not you resolved it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants