-
-
Notifications
You must be signed in to change notification settings - Fork 23.4k
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
Feature: Add gist card #3064
Feature: Add gist card #3064
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3064 +/- ##
==========================================
+ Coverage 97.62% 97.69% +0.06%
==========================================
Files 24 26 +2
Lines 5343 5630 +287
Branches 466 487 +21
==========================================
+ Hits 5216 5500 +284
- Misses 125 128 +3
Partials 2 2
☔ View full report in Codecov by Sentry. |
Perfect. I suppose we can use the same themes as the other cards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request adds new endpoint
http://localhost:3000/api/gist?username=rickstaa&id=1e0f4ebcc7c35bdee2cae9ccf6fec6ec
which generates gist cards. Some examples provided below. What do you think?Perfect. I suppose we can use the same themes as the other cards.
Right, this card supports all themes like the rest. I've updated the pull request description and added some examples. You can check other themes using this preview deploy of this branch: https://github-readme-stats-ksyzpjxl9-github-readme-stats-team.vercel.app/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments. I think using the GraphQL endpoint would make more sense. What do you think?
src/fetchers/gist-fetcher.js
Outdated
const response = ( | ||
await axios({ | ||
method: "get", | ||
url: `https://api.github.com/gists/${id}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get all information from the GraphQL API:
{
viewer {
gist(name: "fb514518d708593b9a218eb61381e400") {
name
owner {
login
}
stargazerCount
forks {
totalCount
}
files{
language {
name
}
size
}
}
}
}
This way, we don't need to add the parseHTML
library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, this way is better since we send only one request instead of two and we don't need additional library for HTML parsing. I have updated the code.
src/fetchers/gist-fetcher.js
Outdated
response.files[Object.keys(response.files)[0]].filename | ||
}`, | ||
description: response.description, | ||
language: response.files[Object.keys(response.files)[0]].language, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you use the language of the first file but a gist can have multiple files. Maybe we can use the language of the file that has the biggest size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote little function for calculating primary language by files size.
Tell me what do you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! I checked your code, and I think it is very well-coded 👍🏻. I guess for consistency; it would be nice:
- If we add a snapshot test (see https://github.com/anuraghazra/github-readme-stats/blob/master/tests/fetchTopLanguages.test.js, for example).
Apart from that, I think this pull request can be merged 🚀!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the comments above 👍🏻. Apart from that, good to go!
@@ -0,0 +1,79 @@ | |||
import { renderGistCard } from "../src/cards/gist-card"; | |||
import { describe, expect, it } from "@jest/globals"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a snapshot test for consistency with the other cards (see https://github.com/anuraghazra/github-readme-stats/blob/master/tests/fetchTopLanguages.test.js for an example for example) 🤔?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I planned to cover new gist card code with tests completely. As you can see i have some todos at the bottom of pull request description and writing tests still actual. Currently i wrote all basic tests including rendering, fetching data and e2e. They works. We can merge this pull request now and increase tests converage to 100% step by step later. Or we can wait for full converage. On your chice, i have converted this pull requests from draft into ready to merge. Feel free to merge.
@qwerty541, thanks for implementing this. Merged into main! |
Awesome! @VydrOz Thank you for great idea suggestion and feedback! @rickstaa Thank you for fast review and useful code improvements suggestions! It was pleasure for me to implement this together with you. |
* Feature: Add gist card * dev * dev * dev * dev * dev * dev * e2e * e2e test timeout
* Feature: Add gist card * dev * dev * dev * dev * dev * dev * e2e * e2e test timeout
* Feature: Add gist card * dev * dev * dev * dev * dev * dev * e2e * e2e test timeout
* Feature: Add gist card * dev * dev * dev * dev * dev * dev * e2e * e2e test timeout
* Feature: Add gist card * dev * dev * dev * dev * dev * dev * e2e * e2e test timeout
This pull request adds new endpoint
https://github-readme-stats-2p1ibrtpu-github-readme-stats-team.vercel.app/api/gist?id=1e0f4ebcc7c35bdee2cae9ccf6fec6ec
which generates gist cards. Some examples provided below. What do you think @rickstaa @francois-rozet @anuraghazra @VydrOz?Preview deployed there:
https://github-readme-stats-2p1ibrtpu-github-readme-stats-team.vercel.app/
TODOS: