Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@Catsudemo
Copy link
Owner

No description provided.

Forgot a critical file!
Created 'displayPage' function and moved event listener into main.
Copy link

@mattiaslundberg mattiaslundberg left a comment

Choose a reason for hiding this comment

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

Approved

Looks good, I have some suggestions for future improvements but nothing you need to fix (think about them for future homeworks).

.then(response => response.json())
.then(data => {
repoDiv.innerHTML = '';
let repository = data.name;

Choose a reason for hiding this comment

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

Always use const instead of let when the value doesn't change.


}

function displayContrib(contributor, contributions, avatar) {

Choose a reason for hiding this comment

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

Nice splitting up in functions!

padding: 5px;
}

.badge {

Choose a reason for hiding this comment

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

This doesn't work well when there are more than 100 contributions, don't forget to test with larger amounts of data!


function displayContrib(contributor, contributions, avatar) {
let eachPersonUl = createAndAppend('ul', contribDiv)
createAndAppend('IMG', eachPersonUl, { src: avatar, width: "42", id: 'avatar' })

Choose a reason for hiding this comment

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

You shouldn't add img elements as a direct child inside ul/ol. Only li is allowed, so you should wrap the img inside of an li.

function displayContrib(contributor, contributions, avatar) {
let eachPersonUl = createAndAppend('ul', contribDiv)
createAndAppend('IMG', eachPersonUl, { src: avatar, width: "42", id: 'avatar' })
createAndAppend('li', eachPersonUl, { class: "badge", id: 'contbadge', text: `${contributions}` })

Choose a reason for hiding this comment

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

Id-attributes must be unique on the entire page, here and on the line above you add multiple elements with the same id. You should use class attribute instead (classes can be used on multiple places in the document).

color: #444;
}

#contributorsDiv.li {

Choose a reason for hiding this comment

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

This doesn't do anything; you're trying to style an element with the id contributorsDiv and the class li. It never exists in your app.

margin: 20px;
}

/* #contributionNumber {

Choose a reason for hiding this comment

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

Don't leave commented out code, remove it instead! (Easier to read and change later)

Fixed a bug that was causing the select element to replicate every time the page reloaded.
Also upperCased everything so it's actually alphabetical
Still haven't figured out how the classes work
Think all is working but api 403 so can't check for a little while.
Checked to make sure this was actually working and removed some unused css.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.