Skip to content

GitHub Tracker Week 7 Louisa#91

Open
Kras053 wants to merge 14 commits into
Technigo:mainfrom
Kras053:main
Open

GitHub Tracker Week 7 Louisa#91
Kras053 wants to merge 14 commits into
Technigo:mainfrom
Kras053:main

Conversation

@Kras053

@Kras053 Kras053 commented Feb 27, 2022

Copy link
Copy Markdown

No description provided.

@mathildakarlsson mathildakarlsson 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 Louisa!

I'm so impressed with how you took on this project and the finished result you got! All of your code-files looks very clean, professional and are easy to follow with great explanations. This project was so hard and you managed to make it work in an amazing way, with a great, structured code. Very very well done!!

Comment thread README.md
And connect the chart to the pullrequests length instead of the forked projects length.

## View it live
https://github-tracker-lo.netlify.app/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What a nicely written ReadMe! Good explanation of the assignment and which techniques you used! Also, it looks really professional with the name of the deployed link since it's not the default one but one that you've created specific for this project!

Comment thread code/chart.js

const myChart = new Chart(document.getElementById('chart'),config);
}

No newline at end of file

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 nice doughnut-chart! I love your choice of colors

Comment thread code/index.html
<script src="./secret.js"></script>
<script src="./script.js"></script>
<script src="./chart.js"></script>
</body>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The HTML-file looks clean and easy to follow!

Comment thread code/index.html
<!-- This will be used to draw the chart 👇 -->
<canvas id="chart"></canvas>
<div class="chart-container">
<canvas class="chart" id="chart"></canvas>

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 is just a very small thing, but you can if you want make an indentation at line 25 since the canvas is inside the div-element, and also an indentation for lines 20 to 26 which are within the main-element

Comment thread code/script.js
Comment on lines +1 to +5
const username = 'Kras053'
const USER_URL =`https://api.github.com/users/${username}`
const REPOS_URL =`https://api.github.com/users/${username}/repos`
const repoContainer = document.querySelector('.repo-container')
const userContainer = document.querySelector('.user-container')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks very neat with these variables at the top of the file :D

Comment thread code/script.js
Comment on lines +87 to +93
if (myPullRequests) {
document.getElementById(`${repo.name}`).innerHTML +=
`<a href="${myPullRequests.html_url}"> Pull request</a>`
} else {
document.getElementById(`${repo.name}`).innerHTML +=
`<p> Pull request made by teammate</p>`
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Awesome that you included your pull requests and put them in conditionals!

Comment thread code/script.js
Comment on lines +95 to +96
//here is a first step to get the commits for each PR, and we add the dynamic id to pass on
// or default message that teammate did the commits

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nicely explained!

Comment thread code/style.css
Comment on lines +1 to +9
* {
margin: 0;
padding: 0;
box-sizing: border-box;
-webkit-box-sizing: border-box;
-moz-box-sizing: content-box;
-webkit-font-smoothing: antialiased;
text-rendering: optimizeLegibility;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great thing that you're adding this, it feel professional :D

Comment thread code/style.css
Comment on lines +58 to +62
.chart-container {
display: grid;
place-items: center;
margin-top: 25px;
}

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 nice positioning of the chart! I struggled with this a lot and your solution looks perfect!

Comment thread code/style.css
Comment on lines +64 to +88
@media (min-width: 668px) {
.repo-container {
grid-template-columns: repeat(2, 1fr);
justify-items: center;
place-items: center;
}
}

@media (min-width: 1024px) {
.repo-container {
grid-template-columns: repeat(3, 1fr);
}

.user-container a:hover {
background-color: black;
color: rgb(217, 96, 152);
}
.user-container img:hover {
border-color:rgb(217, 96, 152);
}

.repo-container a:hover {
background-color: black;
color: rgb(217, 96, 152);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm very impressed with how you used media queries to get such a good responsiveness! I've written a lot of unnecessary code in these and it just won't look good! Your solution with grid is amazing!

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