Skip to content

Github Tracker Maria Petersson#28

Open
hejmaria wants to merge 15 commits into
Technigo:mainfrom
hejmaria:main
Open

Github Tracker Maria Petersson#28
hejmaria wants to merge 15 commits into
Technigo:mainfrom
hejmaria:main

Conversation

@hejmaria

@hejmaria hejmaria commented Oct 3, 2021

Copy link
Copy Markdown

The assignment was to build a GitHub tracker, using APIs to fetch information about our Technigo projects. We needed to filter and find certain bits of the arrays to get the correct information.

It has been a fun, but difficult project. To me, the lessons have been very useful. I can read the code and more or less understand it, but I have a hard time coming up with the correct solutions on my own.
I followed the lessons and then implemented what I had learned in my own code. I always rewrote the code (I didn't copy-paste) in order to grasp what I was doing.
I coded together with my team several times, it was great.

I focused on getting all the js/chart in place first, and then went on with the styling. It was more challenging than I thought, but by asking my team and looking at stack overflow I think it went well!

If I had more time I would look into the red and black requirements. And add a footer with some contact info.

View it live

https://friendly-pike-d9b647.netlify.app

@IdaAspen IdaAspen 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.

Good job Maria!
Your project fulfill all the blue requirements and you managed to write clean code with helpful comments. I am impressed by your styling and css-code, it looked really good and responsive. Time to celebrate 🥳

Comment thread README.md Outdated

Every project should be deployed somewhere. Be sure to include the link to the deployed project so that the viewer can click around and see what it's all about.
## View it live
https://friendly-pike-d9b647.netlify.app 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.

I really like the colors, design and fonts used on your site! It all come together very well and the text and its style with bright color is readable and looks great!

Comment thread code/index.html

<!-- This will be used to draw the chart 👇 -->
<canvas id="chart"></canvas>
<!-- THE PROFILE -->

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 is super clean and tidy ⭐️

Comment thread code/script.js
@@ -0,0 +1,99 @@
/// All API:S ///

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Well commented JS, its easy to follow!

Comment thread code/script.js


/// Fetching the pull requests ///
const fetchPullRequestsArray = (allRepos) => {

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 name of the param to make it easier to read and understand the code

Comment thread code/script.js
.then((data) => {
const userPullRequest = data.find(
(pull) => pull.user.login === repo.owner.login);

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 are adding some extra line breaks here and there in the code, but isn't consistent as I can tell. Maybe that something to think of going forward to make the code even more clean?

Comment thread code/script.js
fetchCommits(userPullRequest.commits_url, repo.name);
} else {
document.getElementById(`commit-${repo.name}`).innerHTML =
`Nope, ${USER} hasn't done a pull request yet.`;

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 add the ${USER} in the else statement!

Comment thread code/style.css
}


/* Responsiveness LARGER PHONE */

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 that you've added larger phone as well!

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