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

Add card_width variable to the repository card #3368 #3359

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

Conversation

airwakz
Copy link

@airwakz airwakz commented Oct 14, 2023

I added the card_width and card_height parameters to the renderRepoCard function, allowing users to specify the card's width and height when calling the function. If these arguments are not provided, it falls back to default values.
Issue Number#2900

@vercel
Copy link

vercel bot commented Oct 14, 2023

@airwakz is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

@airwakz airwakz changed the title Update repo-card.js( Equal height for all pinned repositories) Update repo-card.js( Equal height for all pinned repositories) Issue Number #2900 Oct 14, 2023
@airwakz
Copy link
Author

airwakz commented Oct 14, 2023

@rickstaa here is thep pr please review it

@airwakz
Copy link
Author

airwakz commented Oct 14, 2023

@

@airwakz airwakz closed this Oct 14, 2023
@airwakz airwakz reopened this Oct 14, 2023
@airwakz
Copy link
Author

airwakz commented Oct 14, 2023

@qwerty541 check this please

@airwakz
Copy link
Author

airwakz commented Oct 14, 2023

@rickstaa check it out

src/cards/repo-card.js Outdated Show resolved Hide resolved
@@ -87,8 +89,7 @@ const renderRepoCard = (repo, options = {}) => {
.map((line) => `<tspan dy="1.2em" x="25">${encodeHTML(line)}</tspan>`)
.join("");

const height =
(descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight;
const height = card_height || (descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a minimum card height to prevent the card height from being too small to display the card's content.

Copy link
Author

Choose a reason for hiding this comment

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

what height should i take

Copy link
Author

Choose a reason for hiding this comment

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

`/**

  • Renders repository card details.
  • @param {RepositoryData} repo Repository data.
  • @param {Partial} options Card options.
  • @param {number} card_width The width of the card.
  • @param {number} card_height The height of the card.
  • @returns {string} Repository card SVG object.
    */
    const renderRepoCard = (repo, options = {}, card_width, card_height) => {
    // ... (existing code)

// Calculate a minimum card height to ensure content is visible
const minimumHeight = 150; // You can adjust this value as needed

// Calculate the final card height
const finalHeight = Math.max(
minimumHeight, // Ensure it doesn't go below the minimum height
(descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight
);

// ... (rest of the function)

const card = new Card({
defaultTitle: header.length > 35 ? ${header.slice(0, 35)}... : header,
titlePrefixIcon: icons.contribs,
width: card_width, // Use the calculated card width
height: card_height || finalHeight, // Use the provided card height or the calculated one
border_radius,
colors,
});

// ... (rest of the function)

return card.render(`
${
isTemplate
? // @ts-ignore
getBadgeSVG(i18n.t("repocard.template"), colors.textColor)
: isArchived
? // @ts-ignore
getBadgeSVG(i18n.t("repocard.archived"), colors.textColor)
: ""
}

<text class="description" x="25" y="-5">
  ${descriptionSvg}
</text>

<g transform="translate(30, ${finalHeight - 75})">
  ${starAndForkCount}
</g>

); };

Copy link
Collaborator

Choose a reason for hiding this comment

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

what height should i take

Judging from a repository with only a 1 line description, this height should be 118.8. You can check this out by going to https://github-readme-stats.vercel.app/api/pin/?username=anuraghazra&repo=github-readme-stats and checking out the height of the card using chromes developer tools.

Copy link
Collaborator

Choose a reason for hiding this comment

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

`/**

  • Renders repository card details.
  • @param {RepositoryData} repo Repository data.
  • @param {Partial} options Card options.
  • @param {number} card_width The width of the card.
  • @param {number} card_height The height of the card.
  • @returns {string} Repository card SVG object.
    */
    const renderRepoCard = (repo, options = {}, card_width, card_height) => {
    // ... (existing code)

// Calculate a minimum card height to ensure content is visible const minimumHeight = 150; // You can adjust this value as needed

// Calculate the final card height const finalHeight = Math.max( minimumHeight, // Ensure it doesn't go below the minimum height (descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight );

// ... (rest of the function)

const card = new Card({ defaultTitle: header.length > 35 ? ${header.slice(0, 35)}... : header, titlePrefixIcon: icons.contribs, width: card_width, // Use the calculated card width height: card_height || finalHeight, // Use the provided card height or the calculated one border_radius, colors, });

// ... (rest of the function)

return card.render(` ${ isTemplate ? // @ts-ignore getBadgeSVG(i18n.t("repocard.template"), colors.textColor) : isArchived ? // @ts-ignore getBadgeSVG(i18n.t("repocard.archived"), colors.textColor) : "" }

<text class="description" x="25" y="-5">
  ${descriptionSvg}
</text>

<g transform="translate(30, ${finalHeight - 75})">
  ${starAndForkCount}
</g>

); };

Please apply changes to the pull request. It is hard for me to review pasted code 😅.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should apply the conditions for your minimum height here.

@github-actions github-actions bot added the lang-card Issues related to the language card. label Oct 15, 2023
@rickstaa
Copy link
Collaborator

@airwakz Although I like your enthusiasm for implementing this feature request. Let's take a step back and clarify what was asked in #2900 😅. In this issue, the problem was that @razonyang wanted the ability to set the card width and height of the pin card. For this to be implemented:

  1. First pull request: First, the card_width argument that is already present for the stats and language cards needs to be implemented for the pin card. You can use the code of the stats and language card to see how you can do this.
  2. Second pull request: a new card_height argument must be added. This code will be similar to that of the card_width pull request.

For this, you don't need to touch the code of the language and stats cards. Although another feature request is open for these cards, I assigned @anmolchhabra21 to this (see #3159). If you have any questions, feel free to comment below 😄.

api/top-langs.js Outdated
@@ -16,7 +16,8 @@ export default async (req, res) => {
hide,
hide_title,
hide_border,
card_width,
card_width, // Add card_width and card_height here
Copy link
Collaborator

Choose a reason for hiding this comment

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

@airwakz can you remove these changes? These changes are not related to the pinned card. You should add your changes to https://github.com/anuraghazra/github-readme-stats/blob/master/api/pin.js.

src/cards/repo-card.js Outdated Show resolved Hide resolved
@@ -134,10 +137,13 @@ const renderRepoCard = (repo, options = {}) => {
gap: 25,
}).join("");

// Calculate the card width and height based on the provided arguments or defaults
const width = card_width || 400;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to apply a minimum card width. Similar to is done in the langs and stats cards.

api/top-langs.js Outdated
@@ -84,6 +84,7 @@ export default async (req, res) => {
hide_title: parseBoolean(hide_title),
hide_border: parseBoolean(hide_border),
card_width: parseInt(card_width, 10),
card_height: parseInt(card_height, 10),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not do this in this pull request but do it later in another pull request.

@airwakz
Copy link
Author

airwakz commented Oct 15, 2023

i will make changes tomorrow as I have a college tomorrow .

@rickstaa rickstaa force-pushed the master branch 2 times, most recently from fa36dac to a732a27 Compare October 16, 2023 09:07
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.

@airwakz I reviewed your pull request and found bugs in your code. As a result, I decided to remove the card_height variable for now. I also added the required tests. Please take a look at the changes I made.

You can open a new pull request in which you add the correct code to specify the card height for the repo card. While doing so, please make sure you add the proper tests and test your code locally according to the contribution guide. We can then merge this pull request for adding the card_width variable.

@rickstaa rickstaa changed the title Update repo-card.js( Equal height for all pinned repositories) Issue Number #2900 Add card_width variable to the repository card Oct 16, 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.

@airwakz can you please add the right documentation to the README? See https://github.com/anuraghazra/github-readme-stats?tab=readme-ov-file#stats-card-exclusive-options for an example.

@rickstaa rickstaa changed the title Add card_width variable to the repository card Add card_width variable to the repository card #3368 Oct 16, 2023
@rickstaa
Copy link
Collaborator

This pull request will solve #3368.

@rickstaa rickstaa linked an issue Oct 16, 2023 that may be closed by this pull request
@airwakz
Copy link
Author

airwakz commented Oct 16, 2023

i will add a documentation now

@github-actions github-actions bot added the documentation Improvements or additions to documentation. label Oct 16, 2023
@rickstaa rickstaa added repo-card Issues related to the pin/repo card. and removed repo-card labels Oct 16, 2023
@airwakz
Copy link
Author

airwakz commented Oct 17, 2023

@rickstaa i think i made the changes and the proper dicumentation in code is alerdy provided by you

@rickstaa
Copy link
Collaborator

rickstaa commented Oct 17, 2023

@rickstaa i think i made the changes and the proper dicumentation in code is alerdy provided by you

Like I said in my previous commit, you added the documentation to the wrong section. The documentation should be added to the repo card section (i.e. https://github.com/anuraghazra/github-readme-stats?tab=readme-ov-file#repo-card-exclusive-options). If you could do that, it would be great.

@airwakz
Copy link
Author

airwakz commented Oct 17, 2023

@rickstaa i make the changes in the documentation as requested by uh

@rickstaa
Copy link
Collaborator

@rickstaa i make the changes in the documentation as requested by uh

@airwakz, unfortunately, the documentation still needs to be corrected. You should only add the parameter you added card_width to the https://github.com/anuraghazra/github-readme-stats?tab=readme-ov-file#repo-card-exclusive-options section. Now you have added also the common parameters.

@rickstaa rickstaa mentioned this pull request Oct 18, 2023
@airwakz
Copy link
Author

airwakz commented Oct 18, 2023

I will change it now

@rickstaa rickstaa changed the title Add card_width variable to the repository card #3368 Add card_width variable to the repository card #3368 Oct 30, 2023
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. hacktoberfest-accepted lang-card Issues related to the language card. repo-card Issues related to the pin/repo card. stats-card Feature, Enhancement, Fixes related to stats the stats card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pin card card_width argument
2 participants