-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
feat: Add a display bytes option to top-languages card #3708
base: master
Are you sure you want to change the base?
Conversation
@abap34 is attempting to deploy a commit to the github readme stats Team on Vercel. A member of the Team first needs to authorize it. |
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, @abap34! Thanks for opening this pull request.
I'd like to make some changes in user API. Let's replace display_bytes: boolean
query string parameter with stats_format: "percentages" | "bytes"
.
I think that this way the code will be more scalable. In case if we are going to implement more stats formats for this card in future, like displaying count of repositories where the particular languages was used or count of lines written on particular languages or anything else, then we will be able to extend an existing query string parameter with new enum values instead of bloating the total number of query string parameters with display_repos_count
, display_lines_count
etc.
Thanks for the review, @qwerty541! It would be better to implement it as you recommended. I will make the modifications. By the way, if we implement it this way, I think a display style like "670,404 bytes" can be added. (add options like Displaying the byte count as is is my favorite except that it might break the layout. It's fun to see the number of bytes increase through daily coding. I think it would be nice to have this display option if there is also a shortened version, so that users can choose a shortened one to preserve the layout. But of course, stability of the layout is also important. |
In my opinion it will be better to separate these two features. I'm okay with addition of Less code changes/features per pull request. => More easier and faster review. => More chances to get another collaborators approval and merge. Let's keep this pull request more simple since I expect that layout changes will requre quite more code changes. |
OK, I understand your point. I will implement |
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, @abap34! Thanks for your efforts and patience. I just left a few comments about minor things that needs to be fixed/improved. After these changes I will be ready to give my approval. Special thanks for deployed preview link, this helped to avoid the need to checkout this branch and test locally.
src/cards/top-languages-card.js
Outdated
const progress = parseFloat(((size / totalSize) * 100).toFixed(2)); | ||
const bytes = formatBytes(size); | ||
const showValue = statsFormat === "bytes" ? bytes : `${progress}%`; |
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 see that this file contains the almost same code on 266-269 lines. I think it will be better to avoid duplication of this logic and create separate function.
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'd prefer it that way too, modified at 0c90270.
@@ -7,6 +7,7 @@ import { | |||
parseBoolean, |
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 will be awesome if you can add test of this feature in renderStatsCard.test.js
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 added some tests at 1c85ace
@@ -108,6 +108,7 @@ Please visit [this link](https://give.do/fundraisers/stand-beside-the-victims-of | |||
- [Donut Vertical Chart Language Card Layout](#donut-vertical-chart-language-card-layout) |
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.
You also need to add new query string parameter information in table on 404 line
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.
Sorry, I did not check enough.
I added it at 27da644.
tests/utils.test.js
Outdated
@@ -134,6 +135,20 @@ describe("Test utils.js", () => { | |||
borderColor: "#fff", | |||
}); | |||
}); | |||
|
|||
it("formatBytes: should return expected values", () => { | |||
expect(formatBytes(0)).toBe("0.0 B"); |
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 have some doubts about that bytes have decimal part. I think it should be just 0 B.
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.
Okay. I removed unnecessary decimal parts at 7e17b50
What's Change ?
Add the
&display_bytes=
parameter to the top-languages card.(Fix #3707)
Implementation
display_bytes
parameter toapi/top-langs.js
andsrc/cards/types.d.ts
.render~Layout
function.formatBytes
insrc/common/utils.js
. This function formats the bytes into a human readable stringResult
A preview of this PR can be seen at
https://github-readme-stats-git-featuredisplaybytes-abap34.vercel.app/api/top-langs
.Example: