-
-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
base: master
Are you sure you want to change the base?
Conversation
@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. |
@rickstaa here is thep pr please review it |
@ |
@qwerty541 check this please |
@rickstaa check it out |
src/cards/repo-card.js
Outdated
@@ -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; |
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 add a minimum card height to prevent the card height from being too small to display the card's content.
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.
what height should i take
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.
`/**
- 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>
); };
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.
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.
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.
`/**
- 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 😅.
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 should apply the conditions for your minimum height here.
@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:
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 |
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.
@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
@@ -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; |
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.
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), |
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 not do this in this pull request but do it later in another pull request.
i will make changes tomorrow as I have a college tomorrow . |
Co-authored-by: Rick Staa <[email protected]>
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.
@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.
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.
@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.
This pull request will solve #3368. |
i will add a documentation now |
@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. |
@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 |
I will change it now |
card_width
variable to the repository card #3368
Any plans to merge this? |
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