-
-
Notifications
You must be signed in to change notification settings - Fork 23.1k
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(top langs card): add ability to change progress bar background color in normal layout (resolves #3307) #3312
base: master
Are you sure you want to change the base?
Conversation
…e background of progress bar
@a-s-t-e-y-a is attempting to deploy a commit to the github readme stats Team on Vercel. A member of the Team first needs to authorize it. |
As requested in #3307 |
@rickstaa @anuraghazra @qwerty541 could you please review my PR |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3312 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 27 27
Lines 5931 5950 +19
Branches 526 527 +1
=======================================
+ Hits 5835 5854 +19
Misses 93 93
Partials 3 3
☔ View full report in Codecov by Sentry. |
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.
Hey, @a-s-t-e-y-a! Thanks for your pull request! Generally your code is well, i'm ready to give my approval when requested changes will be done. Also it will be good if you can cover this feature with test. In case if you can provide a link on test deployment to be sure that it works, i'm okay with adding test in future pull request.
@@ -143,7 +143,7 @@ Change the `?username=` value to your GitHub username. | |||
> By default, the stats card only shows statistics like stars, commits and pull requests from public repositories. To show private statistics on the stats card, you should [deploy your own instance](#deploy-on-your-own) using your own GitHub API token. | |||
|
|||
> [!NOTE]\ | |||
> Available ranks are S (top 1%), A+ (12.5%), A (25%), A- (37.5%), B+ (50%), B (62.5%), B- (75%), C+ (87.5%) and C (everyone). This ranking scheme is based on the [Japanese academic grading](https://wikipedia.org/wiki/Academic_grading_in_Japan) system. The global percentile is calculated as a weighted sum of percentiles for each statistic (number of commits, pull requests, reviews, issues, stars and followers), based on the cumulative distribution function of the [exponential](https://wikipedia.org/wiki/exponential_distribution) and the [log-normal](https://wikipedia.org/wiki/Log-normal_distribution) distributions. The implementation can be investigated at [src/calculateRank.js](./src/calculateRank.js). The circle around the rank shows 100 minus the global percentile. | |||
> Available ranks are S (top 1%), A+ (12.5%), A (25%), A- (37.5%), B+ (50%), B (62.5%), B- (75%), C+ (87.5%) and C (everyone). This ranking scheme is based on the [Japanese academic grading](https://wikipedia.org/wiki/Academic_grading_in_Japan) system. The global percentile is calculated as a weighted sum of percentiles for each statistic (number of commits, pull requests, reviews, issues, stars and followers), based on the cumulative distribution function of the [exponential](https://wikipedia.org/wiki/exponential_distribution) and the [log-normal](https://wikipedia.org/wiki/Log-normal_distribution) distributions. The implementation can be investigated at [src/calculateRank.js](https://github.com/anuraghazra/github-readme-stats/blob/master/src/calculateRank.js). The circle around the rank shows 100 minus the global percentile. |
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.
Your pull request should contain changes only related to linked issue, you should remove this. Instead of this you should add information about new query parameter into language card options section https://github.com/anuraghazra/github-readme-stats#language-card-exclusive-options.
} = options; | ||
|
||
console.log(prog_bg_color); |
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 console log should be removed
@@ -33,9 +33,9 @@ export default async (req, res) => { | |||
border_color, | |||
disable_animations, | |||
hide_progress, | |||
prog_bg_color, |
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.
Let's rename it to progress_bar_bg_color
or prog_bar_bg_color
everywhere for consistency.
@@ -322,9 +332,10 @@ const createDonutLanguagesNode = ({ langs, totalSize }) => { | |||
* @param {Lang[]} langs Array of programming languages. | |||
* @param {number} width Card width. | |||
* @param {number} totalLanguageSize Total size of all languages. | |||
* @param {string} prog_bg_color Total size of all languages. |
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.
Description of this function argument was copy-pasted from previous. You should write correct description. Also type of argument does not indicate that it can be undefined
.
* @returns {string} Normal layout card SVG object. | ||
*/ | ||
const renderNormalLayout = (langs, width, totalLanguageSize) => { | ||
const renderNormalLayout = (langs, width, totalLanguageSize, prog_bg_color) => { |
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.
It's better to use camel case for this function argument.
@@ -203,11 +203,19 @@ const trimTopLanguages = (topLangs, langs_count, hide) => { | |||
* @param {number} props.width The card width | |||
* @param {string} props.color Color of the programming language. | |||
* @param {string} props.name Name of the programming language. | |||
* @param {string} props.prog_bg_color Color of the background of progress bar |
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.
Type of this argument does not indicate that it can be undefined
.
@@ -6,6 +6,7 @@ export type CommonOptions = { | |||
icon_color: string; | |||
text_color: string; | |||
bg_color: string; | |||
prog_bg_color: string; |
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.
It is wrong place for adding this parameter since it is exclusive for top languages card. It should be moved to TopLangOptions
. Most likely that it is the reason why you did not get type error missing undefined
variant in previous comments.
name, | ||
progress, | ||
index, | ||
prog_bg_color, |
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.
It's better to use camel case for this function argument.
@a-s-t-e-y-a could you please change everything requested? |
"prog_bg_color" query added to change the background of progress bar