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

feat: Add a display bytes option to top-languages card #3708

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

abap34
Copy link

@abap34 abap34 commented Mar 25, 2024

What's Change ?

Add the &display_bytes= parameter to the top-languages card.

(Fix #3707)

Implementation

  1. Add display_bytes parameter to api/top-langs.js and src/cards/types.d.ts.
  2. Pass this parameter to each render~Layout function.
  3. Implement formatBytes in src/common/utils.js. This function formats the bytes into a human readable string
  4. Write the following code so that the appropriate values are displayed where the progress is actually generated.
const percentage = ((lang.size / totalSize) * 100).toFixed(2) + "%";
const bytes = formatBytes(lang.size);

const showValue = displayBytes ? bytes : percentage;

Result

A preview of this PR can be seen at https://github-readme-stats-git-featuredisplaybytes-abap34.vercel.app/api/top-langs.

Example:

Copy link

vercel bot commented Mar 25, 2024

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

@github-actions github-actions bot added documentation Improvements or additions to documentation. lang-card Issues related to the language card. labels Mar 25, 2024
Copy link
Collaborator

@qwerty541 qwerty541 left a 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.

@abap34
Copy link
Author

abap34 commented Mar 26, 2024

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 bytes_short, bytes)

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.
What do you think about this idea?

@qwerty541
Copy link
Collaborator

In my opinion it will be better to separate these two features. I'm okay with addition of bytes stats format for now and bytes_long in future. I always try to adhere the following principle:

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.

@abap34
Copy link
Author

abap34 commented Apr 3, 2024

OK, I understand your point.

I will implement bytes and percentages in this PR.

@abap34
Copy link
Author

abap34 commented Apr 3, 2024

I have done the implementation.
(https://github-readme-stats-git-featuredisplaybytes-abap34s-projects.vercel.app/api/top-langs/?username=anuraghazra&stats_format=bytes to preview!)

The only change I made was to replace all displayBytes with statsFormat and error-checking.

Copy link
Collaborator

@qwerty541 qwerty541 left a 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.

Comment on lines 227 to 229
const progress = parseFloat(((size / totalSize) * 100).toFixed(2));
const bytes = formatBytes(size);
const showValue = statsFormat === "bytes" ? bytes : `${progress}%`;
Copy link
Collaborator

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.

Copy link
Author

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,
Copy link
Collaborator

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

Copy link
Author

@abap34 abap34 Apr 13, 2024

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)
Copy link
Collaborator

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

Copy link
Author

@abap34 abap34 Apr 13, 2024

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.

@@ -134,6 +135,20 @@ describe("Test utils.js", () => {
borderColor: "#fff",
});
});

it("formatBytes: should return expected values", () => {
expect(formatBytes(0)).toBe("0.0 B");
Copy link
Collaborator

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.

Copy link
Author

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

@abap34 abap34 requested a review from qwerty541 April 13, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation. feature lang-card Issues related to the language card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to display byte count for Top-languages
2 participants