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

Creates a new Streaks Card #3338

Closed
wants to merge 13 commits into from
Closed

Creates a new Streaks Card #3338

wants to merge 13 commits into from

Conversation

yaten2302
Copy link

Fixes issue - #3209

This PR creates a new streak card which shows the user's streak on GitHub.

Screenshots

image


image

@vercel
Copy link

vercel bot commented Oct 9, 2023

@yaten2302 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. card-i18n Card text translations. labels Oct 9, 2023
Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

@yaten2302 First, you did an excellent job on your first PR. You can be genuinely proud of yourself 🚀. I added some things that need to be improved before this pull request can be merged. Besides these points, the pull requests also lack tests, which need to be added. You can check out the tests folder to see how these tests should be implemented.

I also like the simplicity of the design of your card. I think when icons are added, it truly gives users a different card than the one already provided by https://github.com/DenverCoder1/github-readme-streak-stats ❤️‍🔥. One improvement I can think of is to add a small graph to the right of the stats that displays the user's contributions over time (see https://github.com/Ashutosh00710/github-readme-activity-graph) 🤔.

image

api/streak.js Show resolved Hide resolved
api/streak.js Show resolved Hide resolved
api/streak.js Show resolved Hide resolved
@@ -568,6 +571,24 @@ Change the `?username=` value to your [Wakatime](https://wakatime.com) username.

***
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
***

### Hide individual elements from the streaks card

To hide individual elements form the card, use - `&hide=params`, where `params` = `totalContributions`, `weeklyLongest`, `weeklyCurrent`, `dailyLongest`, `dailyCurrent`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
***

src/cards/streak-card.js Show resolved Hide resolved
@@ -123,13 +123,23 @@ const iconWithLabel = (icon, label, testid, iconSize) => {
/**
* Retrieves num with suffix k(thousands) precise to 1 decimal if greater than 999.
*
* @param {number} num The number to format.
* @returns {string|number} The formatted number.
* num The number to format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are the typings removed 🤔 ?

Copy link
Author

Choose a reason for hiding this comment

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

I guess, I removed those typings by mistake, will add those again 👍

: Math.sign(num) * Math.abs(num);
: Math.sign(num) * Math.abs(num); */

if (typeof num == "number") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to be changed? This function should only accept numbers. In its current form strings should be converted before they enter the 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 changed this because while rendering the streaks card, it requires some additional information in the value (like - days or weeks)

image
If this is reverted to the original code, then it will show NaN in the value instead of 2 weeks or something.
This change will not affect the other cards. But, if you want, then I can make a separate function in the utils.js specifically for the Streaks card?

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. You should handle this problem outside the kFormatter function. Maybe only use the kformatter function when the value is a number:

const kValue = number_format === "long" ? value : kFormatter(value);

src/fetchers/streak-fetcher.js Show resolved Hide resolved
src/fetchers/streak-fetcher.js Show resolved Hide resolved
@rickstaa
Copy link
Collaborator

Since my comments will take you some time to implement, I already attached the hacktoberfest-accepted label so that this pull request will appear in your hacktoberfest portal 👍🏻.

@yaten2302
Copy link
Author

Hi @rickstaa, thanks for your valuable suggestions, I'll surely make the necessary changes ASAP.
Also, I would like to state, that I had a similar idea of creating a Graph of user's contributions, as mentioned by you. I thought of creating a separate issue for, I thought that the Graph would look odd in the Streaks card?

So, should I create a separate card for the graph or should I add it here only?

@rickstaa
Copy link
Collaborator

rickstaa commented Oct 14, 2023

Hi @rickstaa, thanks for your valuable suggestions, I'll surely make the necessary changes ASAP. Also, I would like to state, that I had a similar idea of creating a Graph of user's contributions, as mentioned by you. I thought of creating a separate issue for, I thought that the Graph would look odd in the Streaks card.

So, should I create a separate card for the graph or add it here?

Or something like this

image

Which I quickly created in GIMP. Coding it would be more work, though. But maybe you can combine the code in https://github.com/Ashutosh00710/github-readme-activity-graph/tree/main/src with the code you already have 🤔.

@qwerty541, @anuraghazra, what are your thoughts on this design and whether we should add a new streak card in the GRS repository?

@anuraghazra
Copy link
Owner

what are your thoughts on this design and whether we should add a new streak card in the GRS repository?

Will have to think this through, I'm not sure about adding this Card.

  1. There are already solutions out there which have better design & more features and specific to streaks Card
  2. Not sure if it would be healthy to add streaks Card both in terms of open source morality & the monopoly, I find it a bit conflicting in one hand github-readme-stats is probably the most popular stats card service out there and I want other stats card service like readme-activity-tree & github-readme-streaks-card to also be part of the community instead of us having all the Cards and being a monopoly.
  3. More cards -> more maintenance -> more vercel load

@rickstaa
Copy link
Collaborator

what are your thoughts on this design and whether we should add a new streak card in the GRS repository?

Will have to think this through, I'm not sure about adding this Card.

  1. There are already solutions out there which have better design & more features and specific to streaks Card
  2. Not sure if it would be healthy to add streaks Card both in terms of open source morality & the monopoly, I find it a bit conflicting in one hand github-readme-stats is probably the most popular stats card service out there and I want other stats card service like readme-activity-tree & github-readme-streaks-card to also be part of the community instead of us having all the Cards and being a monopoly.
  3. More cards -> more maintenance -> more vercel load

@anuraghazra, thanks for your thoughts. Those are excellent points, and I think it is similar to the statement by @qwerty541 that we should only add this Card if it has a unique design/feature set compared to https://github.com/DenverCoder1/github-readme-streak-stats (see #3209 (comment)). Sadly, the current implementation by @yaten2302, in my opinion, is not unique enough. I think my design, which combines https://github.com/Ashutosh00710/github-readme-activity-graph and https://github.com/DenverCoder1/github-readme-streak-stats, would offer a special card for users. Neverteless, this Card might be best realised as a feature/pull request to https://github.com/DenverCoder1/github-readme-streak-stats instead of being added to GRS.

@rickstaa
Copy link
Collaborator

what are your thoughts on this design and whether we should add a new streak card in the GRS repository?

Will have to think this through, I'm not sure about adding this Card.

  1. There are already solutions out there which have better design & more features and specific to streaks Card
  2. Not sure if it would be healthy to add streaks Card both in terms of open source morality & the monopoly, I find it a bit conflicting in one hand github-readme-stats is probably the most popular stats card service out there and I want other stats card service like readme-activity-tree & github-readme-streaks-card to also be part of the community instead of us having all the Cards and being a monopoly.
  3. More cards -> more maintenance -> more vercel load

@anuraghazra, thanks for your thoughts. Those are excellent points, and I think it is similar to the statement by @qwerty541 that we should only add this Card if it has a unique design/feature set compared to https://github.com/DenverCoder1/github-readme-streak-stats (see #3209 (comment)). Sadly, the current implementation by @yaten2302, in my opinion, is not unique enough. I think my design, which combines https://github.com/Ashutosh00710/github-readme-activity-graph and https://github.com/DenverCoder1/github-readme-streak-stats, would offer a special card for users. Neverteless, this Card might be best realised as a feature/pull request to https://github.com/DenverCoder1/github-readme-streak-stats instead of being added to GRS.

Originally posted by @rickstaa in #3338 (comment)

@rickstaa
Copy link
Collaborator

@qwerty541, @anuraghazra Let's continue the discussion on #3338.

@yaten2302
Copy link
Author

@rickstaa, @anuraghazra, @qwerty541, is this card not required or it requires a change in its design?

@rickstaa
Copy link
Collaborator

@rickstaa, @anuraghazra, @qwerty541, is this card not required, or it requires a change in its design?

To summarize. We concluded that your card doesn't offer more new features compared to https://github.com/DenverCoder1/github-readme-streak-stats. Adding it to GRS might create a monopoly against https://github.com/DenverCoder1/github-readme-streak-stats since this project is more popular 😅. Maybe @anuraghazra can elaborate on whether he wants to accept this card if it has a very different design from https://github.com/DenverCoder1/github-readme-streak-stats.

@anuraghazra
Copy link
Owner

Conclusion: let's refrain from adding Card. Given there are already better solutions out there which people can use, plus it reduces our maintenance and dev efforts.

@rickstaa
Copy link
Collaborator

Conclusion: let's refrain from adding Card. Given there are already better solutions out there which people can use, plus it reduces our maintenance and dev efforts.

I agree. Let's close this when Hacktoberfest is over 👍🏻.

@yaten2302 yaten2302 closed this by deleting the head repository Oct 22, 2023
@qwerty541 qwerty541 mentioned this pull request Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
card-i18n Card text translations. documentation Improvements or additions to documentation. gist-card hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants