Skip to content

Week 7 - Project-github-tracker -Jessi Nygren Walhed#40

Open
hemmahosjessi wants to merge 23 commits intoTechnigo:mainfrom
hemmahosjessi:main
Open

Week 7 - Project-github-tracker -Jessi Nygren Walhed#40
hemmahosjessi wants to merge 23 commits intoTechnigo:mainfrom
hemmahosjessi:main

Conversation

@hemmahosjessi
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@Ajliin Ajliin left a comment

Choose a reason for hiding this comment

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

Such a brilliant job! The site looks overall very professional you have such a good eye for design and layout! The responsiveness works perfect and the code is clean and structured. You have met all requirements!

I really tried to find some things on to improve, but it was a hard job! I have made some small comments in code. I have learnt a lot reading you code, big thank you! (Css 471 rows :o)

A nice thing to add would be a specific name on the netlify-site (easy to change in netlify under site-settings —> change site name) And maybe a logo in the tab :)

.main {
display: flex;
flex-direction: row;
padding-left: 5%;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

padding: 0 5%; would be the same (save one row code :)

.then(response => (response.json()))
.then(data => {
const forkedRepos = data.filter(repo => repo.fork && repo.name.startsWith('project-'))
console.log('My forked repos', forkedRepos)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

console.log - love them, but they got to go

console.log('My forked repos', forkedRepos)

forkedRepos.forEach(repo => projectsContainer.innerHTML += `
<div class="project-card">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice to see all the styling in the innerHTML, good job!

.then(data => {
const myPullRequests = data.find(
(pull) => pull.user.login === repo.owner.login)
console.log('My Pull requests', myPullRequests)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

cleaning

fetch(myCommitsUrl)
.then(response => (response.json()))
.then(data => {
console.log('My commits', data)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

console..


getRepos()


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You use very little comments for the js-code, it might help when u going back to this project later on if you have written some more comments on what u have done.. Just an idea

transform-origin: bottom left;
}

a::before {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice!

@media (min-width: 768px) and (max-width: 991px) {

.main {
display: flex;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

display flex is already in mobile first

}

.left-area {
box-sizing: border-box;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this should be uneccesary if u have it in html?

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.

2 participants