Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitHub stats to the status dashboard #2466

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Nov 13, 2021

Issue This PR Addresses

Fixes #2417

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR adds GitHub stats for Telescope and Satellite.
Stats available: yearly/weekly number of commits, weekly lines added/removed and latest contributor.

Screen Shot 2021-11-13 at 6 02 04 PM

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 13, 2021

@menghif
Copy link
Contributor Author

menghif commented Nov 13, 2021

I haven't found a way to get the total number of contributors since Github only returns a max of 100:
https://docs.github.com/en/rest/reference/repos#list-repository-contributors
Any tips on how to get this?

@humphd
Copy link
Contributor

humphd commented Nov 14, 2021

Request 1 item per page, and then look at the last page you're given for pagination in the link header they send in the response:

 curl -I -H "Accept: application/vnd.github.v3+json" "https://api.github.com/repos/Seneca-CDOT/telescope/contributors?per_page=1&anon=true"
HTTP/2 200
server: GitHub.com
date: Sat, 13 Nov 2021 23:55:18 GMT
content-type: application/json; charset=utf-8
...
link: <https://api.github.com/repositories/217872944/contributors?per_page=1&anon=true&page=2>; rel="next", <https://api.github.com/repositories/217872944/contributors?per_page=1&anon=true&page=153>; rel="last"
...

I think we have some other code that parses this, which you can steal.

Hey, I love what you're doing above! Some initial thoughts

  • Should we show something like "No commits this week" instead of "0" for commits, lines added, etc. Let's find a way to deal with slow periods, of which there are many
  • Can you make as many things as possible into links? e.g., last contributor could link to the person. We could also grab their avatar image and show it (https://github.com/{user}.png)

Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

This is awesome!

It may look better with all the cards having the same height. Suck that I don't have a better way to show you my changes, but here it is.

  1. Add height:100% to the tag with class card
  2. Add justify-content: space-between to the tag with class card
  3. Add margin-bottom: auto to the tag with class card-header, or margin-top: auto to the hr.

image

src/api/status/public/index.html Outdated Show resolved Hide resolved
@menghif
Copy link
Contributor Author

menghif commented Nov 15, 2021

@humphd Thanks for the help on getting the total contributors!
I updated the "Last contributor" section and added the user's avatar (as link to the github profile). I also made the changes suggested by @Andrewnt219

Screen Shot 2021-11-15 at 4 46 35 PM

@menghif menghif marked this pull request as ready for review November 15, 2021 21:57
src/api/status/public/assets/js/gitHubStats.js Outdated Show resolved Hide resolved
let weeklyCommits = 0;

// get weekly commits for last year: https://docs.github.com/en/rest/reference/repos#get-the-weekly-commit-count
fetch(`https://api.github.com/repos/Seneca-CDOT/${repo}/stats/participation`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Write a function to build your GitHub API urls for you vs. hard-coding below:

const ghApiUrl = (repo = 'Seneca-CDOT', path) => `https://api.github.com/repos/Seneca-CDOT/${repo}{path}`

Then you can use it like fetch(ghApiUrl('/stats/participation))

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading further down, you can actually reuse a bunch of code here:

const fetchGitHubApi = (repo = 'Seneca-CDOT', path) =>
  fetch(`https://api.github.com/repos/${repo}{path}`, {
    headers: {  Accept: 'application/vnd.github.v3+json', }
  })
  .then(res => {
    if(!res.ok) {
      throw new Error(`unable to get ${path}`);
    }
    return res.json()
  )
  .catch(err => console.error(err));

Now you can do:

fetchGitHubApi('/stats/participation')
  .then(data => { /* process data here */);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's much better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't reuse some parts because I don't always return res.json().

src/api/status/public/assets/js/gitHubStats.js Outdated Show resolved Hide resolved
@@ -1222,6 +1284,7 @@ <h6 class="mt-3">Thank you for sharing!</h6>
<script src="assets/js/plugins/chartjs.min.js"></script>
<script src="assets/js/demo-chart.js"></script>
<script src="assets/js/demo-sidenav.js"></script>
<script src="assets/js/gitHubStats.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a section specifically for our Telescope stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inside assets/js? What should I call it?

connectSrc: [
"'self'",
'*.fontawesome.com',
`${process.env.API_HOST.replace(/(^\w+:|^)\/\//, '')}:4000`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this fixed in another PR already? cc @manekenpix

src/api/status/public/index.html Outdated Show resolved Hide resolved
src/api/status/public/index.html Outdated Show resolved Hide resolved
@@ -5006,6 +5006,8 @@ fieldset:disabled .btn {
position: relative;
display: flex;
flex-direction: column;
height: 100%;
Copy link
Contributor

@Andrewnt219 Andrewnt219 Nov 16, 2021

Choose a reason for hiding this comment

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

I don't recommend overriding styles in the default asset, as it will affect other components. You can add another stylesheet to override the default.

This will be merged with the base style...

/* homepage.css */
.card {
  height: 100%;
  justify-content: space-between
}

...or make another class to make it clear (BEM style)

/* homepage.css */
.card--dashboard {
  height: 100%;
  justify-content: space-between
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put telescope- in the name somehow, and maybe in a different directory, so it's clear that we are overriding the upstream styles.

Copy link
Contributor

@Andrewnt219 Andrewnt219 Nov 16, 2021

Choose a reason for hiding this comment

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

Let's put it in /public/css/homepage.css, and import it after the default assets.

For the class name, .card--dashboard should be clear about the override. Breakdown: "card" is the component, "dashboard" is the modifier. The modifier indicates that it must be used with the "card" class and goes after the "card" class (i.e. className="card card--dashboard"). This follows the popular BEM methodology, which makes it easy for new contributors to follow.

EDIT: "dashboard" because these are changes for the cards in the dashboard only. If we use the cards somewhere else in the homepage, we may not need this "dashboard" modifier.

@humphd
Copy link
Contributor

humphd commented Nov 16, 2021

This needs a rebase too, to pick up the changes in server.js

@menghif
Copy link
Contributor Author

menghif commented Nov 16, 2021

I rebased and moved the js and css files based on the recommendations

@humphd humphd requested a review from suhhee1011 November 16, 2021 23:32
Andrewnt219
Andrewnt219 previously approved these changes Nov 17, 2021
Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

Awesome, looking forward to seeing it live.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise this looks ready to go.

@@ -0,0 +1,68 @@
const fetchGitHubApi = (owner, repo, path) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If your arrow function immediately returns the result of an expression, you don't need a body or return:

const fetchGitHubApi = (owner, repo, path) =>
  fetch(`https://api.github.com/repos/${owner}/${repo}/${path}`, {
    headers: { Accept: 'application/vnd.github.v3+json' },
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@humphd humphd merged commit 3131812 into Seneca-CDOT:master Nov 17, 2021
@menghif menghif deleted the issue-2417 branch December 8, 2021 01:46
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.

Add GitHub stats to our status dashboard
4 participants