Skip to content

GitHub Tracker - Arianna#86

Open
ariallahyar wants to merge 20 commits into
Technigo:mainfrom
ariallahyar:main
Open

GitHub Tracker - Arianna#86
ariallahyar wants to merge 20 commits into
Technigo:mainfrom
ariallahyar:main

Conversation

@ariallahyar

Copy link
Copy Markdown

Here's a link to my project live: https://githubtracker-ari.netlify.app/

@sieurin sieurin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Ari,

Such a well done job on your git hub tracker! Your site works as intended in all screen sizes and different browsers. There are no bugs. Your project has fulfilled all the blue and general requirements and you seem to have met the red level requirements as well. You should be so proud of yourself!

Your code is well organized but the most impressive thing with your code is that it’s so condensed and clean. It’s really a joy to read it.

I have just made some small comments, merely suggestions. So, I will keep this code review as short as your code :)

Comment thread code/script.js
Comment on lines +17 to +29
const fetcher = (url, token, callback) => {
fetch(url, token)
.then((response) => {
return response.json();
})
.then((data) => {
callback(data);
})
.catch((error) => {
console.log(error);
})
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Love this! Makes the code so much cleaner below. Will ask you more questions about this :)

Comment thread code/style.css
background-color: rgb(63, 203, 226, 0.15);
padding: 1rem 1rem;
margin: 1rem;
line-height: 22px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there any reason why you use pixels sometimes and rem sometimes?

Comment thread code/style.css
padding: 1rem 0rem;
}

.profile-wrapper {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very clear and descriptive css naming conventions in general!

Comment thread code/script.js
};

fetcher (url.user, OPTIONS, ((userProfile) => {
document.getElementById('profile').innerHTML += `

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you can add some comments to your code. It's very easy to follow, because of your clean code, but I think comments is a good thing to make it even more easy to follow along with your code.

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