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

[Proposal] Equal height for all pinned repositories #2900

Closed
razonyang opened this issue Jul 1, 2023 · 26 comments · Fixed by #3453
Closed

[Proposal] Equal height for all pinned repositories #2900

razonyang opened this issue Jul 1, 2023 · 26 comments · Fixed by #3453
Assignees
Labels
enhancement New feature or request. hacktoberfest repo-card Issues related to the pin/repo card. ⭐ top feature Top feature request. ⭐ top issue Top issue.

Comments

@razonyang
Copy link

razonyang commented Jul 1, 2023

Is your feature request related to a problem? Please describe.

Hi, I found that the pinned repos are not equal height.
image

Describe the solution you'd like

GitHub built-in:
image

Describe alternatives you've considered

No response

Additional context

No response

@razonyang razonyang added the enhancement New feature or request. label Jul 1, 2023
@rickstaa
Copy link
Collaborator

rickstaa commented Jul 3, 2023

@razonyang, thanks for your bug report. This is the intended behaviour since we want the card to resize to the repository description automatically, and users are expected to handle the card height themselves using the HTML height attribute. The GitHub pin card this card was based on handling the height change on the profile page.

image

So I think I prefer to close this issue. However, if @qwerty541 and @anuraghazra think this behaviour is not intended, we can change the minimum size of the repo/pin card to show at least two lines.

<a href="https://github.com/anuraghazra/github-readme-stats">
  <img height="140" align="center" src="https://github-readme-stats.vercel.app/api/pin/?username=anuraghazra&repo=github-readme-stats" />
</a>
<a href="https://github.com/anuraghazra/convoychat">
  <img align="center" src="https://github-readme-stats.vercel.app/api/pin/?username=rickstaa&repo=github-emoji-picker" />
</a>

@razonyang
Copy link
Author

@rickstaa Thanks for the code snippet, but the fixed height doesn't look good, equal width and height looks better. The font-size are also smaller than others.

image

It may better that if the description can take 2 rows, fill up with whitespace if not enough. Please feel free to close it if it's not possible or reasonable. I've solved it by modifying the descriptions.

@rickstaa
Copy link
Collaborator

rickstaa commented Jul 3, 2023

@rickstaa Thanks for the code snippet, but the fixed height doesn't look good, equal width and height looks better. The font-size are also smaller than others.

image

It may better that if the description can take 2 rows, fill up with whitespace if not enough. Please feel free to close it if it's not possible or reasonable. I've solved it by modifying the descriptions.

Thanks for your feedback. We have the card_width option to change the card width. One way to improve this behaviour is to add a card_height option. Freel free to create a pull request 👍🏻.

@rickstaa rickstaa closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2023
@rickstaa
Copy link
Collaborator

rickstaa commented Jul 3, 2023

@rickstaa Thanks for the code snippet, but the fixed height doesn't look good, equal width and height looks better. The font-size are also smaller than others.
image
It may better that if the description can take 2 rows, fill up with whitespace if not enough. Please feel free to close it if it's not possible or reasonable. I've solved it by modifying the descriptions.

Thanks for your feedback. We have the card_width option to change the card width. One way to improve this behaviour is to add a card_height option. Freel free to create a pull request 👍🏻.

It looks like my previous comment was incorrect 🤔. The card_width argument has not yet been implemented for the pin-card. Feel free to create a pull request or open a new pull request to have this argument and the card_height argument implemented.

@rickstaa rickstaa reopened this Jul 3, 2023
@rickstaa
Copy link
Collaborator

rickstaa commented Jul 3, 2023

I re-opened this issue so that @qwerty541 and @anuraghazra can give their opinion.

@qwerty541
Copy link
Collaborator

I re-opened this issue so that @qwerty541 and @anuraghazra can give their opinion.

@rickstaa Maybe this behavior is good one to be improved but i think i will be very hard to do working with pure SVG. We can return to this after #1633 will be implemented.

@rickstaa
Copy link
Collaborator

I re-opened this issue so that @qwerty541 and @anuraghazra can give their opinion.

@rickstaa Maybe this behavior is good one to be improved but i think i will be very hard to do working with pure SVG. We can return to this after #1633 will be implemented.

I agree let's keep this open for now 👍🏻.

@github-actions github-actions bot added the ⭐ top feature Top feature request. label Jul 31, 2023
@github-actions github-actions bot added the ⭐ top issue Top issue. label Sep 18, 2023
@github-actions github-actions bot removed the ⭐ top issue Top issue. label Sep 28, 2023
@airwakz
Copy link

airwakz commented Oct 13, 2023

@rickstaa can i do this task and can uh set it for hacktoberfest

@rickstaa
Copy link
Collaborator

@airwakz of course. Thanks for choosing us for the Hacktoberfest! You can implement the card_width and card_height arguments for the pin (i.e. repo) card 👍🏻.

@airwakz
Copy link

airwakz commented Oct 14, 2023

`

<style> .repository-cards { display: flex; flex-wrap: wrap; justify-content: center; } .repository-card { width: 300px; /* Set the desired width for each card */ height: 200px; /* Set the desired height for each card */ margin: 10px; /* Add margin between cards if needed */ } </style>

`

@airwakz
Copy link

airwakz commented Oct 14, 2023

ig thanks for the detailed response. I successfully replicated the issue you mentioned. It seems to stem from the use of the HTML height property, causing the SVG image to scale and the fonts to enlarge.

I'm thinking we could introduce a card_height argument, ensuring that it maintains the minimum height required for displaying the text 🤔. Any thoughts on this, @anuraghazra or @qwerty541? Open to better ideas!

Without height property

`

` With Height properly ` <style> .repository-card { width: 300px; /* Set the desired width for each card */ height: 200px; /* Set the desired height for each card */ margin: 10px; /* Add margin between cards if needed */ } </style>`

@airwakz
Copy link

airwakz commented Oct 14, 2023

@rickstaa do uh want me to make any changes to

@rickstaa
Copy link
Collaborator

@airwakz, your conclusion is correct (see #3159 (comment)). The same problem occurs with the other cards in #3159. Adding the card_width and card_height parameters to the pin card will solve the problem. #2900 (comment)

I already assigned @anmolchhabra21 to solve this problem for the cards. I just asked if he is still working on that issue. In the meantime, you can add the card_width parameter to the pin card and create a separate pull request for that (see #2900 (comment)).

@airwakz
Copy link

airwakz commented Oct 14, 2023

@rickstaa can uh tell me in which file i need to make changes as i am unable to make changes in the file and a pr need a change of html code or a file

@rickstaa
Copy link
Collaborator

rickstaa commented Oct 14, 2023

Yea, no problem. To implement the card_width argument, you need to add the card_width code found in the stats card (i.e. https://github.com/anuraghazra/github-readme-stats/blob/master/src/cards/stats-card.js) to the pin card (i.e. https://github.com/anuraghazra/github-readme-stats/blob/master/src/cards/repo-card.js). It would be mainly JavaScript code. You can check out the https://github.com/anuraghazra/github-readme-stats/blob/master/CONTRIBUTING.md#local-development guide on how you can run and debug your changes on your local machine. If you run into problems, feel free to comment below 👍🏻.

@rickstaa
Copy link
Collaborator

@airwakz I just noticed that the contributing guide is outdated and created a pull request -> https://github.com/anuraghazra/github-readme-stats/pull/3358/files. You can use the steps in this pull request.

@airwakz
Copy link

airwakz commented Oct 14, 2023

`/ Import the card_width logic from the "stats card"
import { CARD_MIN_WIDTH as REPO_CARD_MIN_WIDTH, CARD_DEFAULT_WIDTH as REPO_CARD_DEFAULT_WIDTH } from "./stats-card.js";

// ... (Existing code)

// Define your own minCardWidth and defaultCardWidth for the "repo card"
const REPO_CARD_ICON_SIZE = 16; // Adjust this value as needed
const REPO_CARD_MIN_WIDTH = /* Calculate the minimum width for the repo card /;
const REPO_CARD_DEFAULT_WIDTH = /
Calculate the default width for the repo card */;

/**

  • Check the card_width option and set the card width accordingly.
    */
    const cardWidth = options.card_width
    ? isNaN(options.card_width)
    ? REPO_CARD_DEFAULT_WIDTH
    : Math.max(REPO_CARD_MIN_WIDTH, options.card_width) // Ensure it doesn't go below the minimum width
    : REPO_CARD_DEFAULT_WIDTH;

// ... (Existing code)

/**

  • Create the Card instance with the calculated cardWidth.
    */
    const card = new Card({
    defaultTitle: header.length > 35 ? ${header.slice(0, 35)}... : header,
    titlePrefixIcon: icons.contribs,
    width: cardWidth, // Use the calculated card width
    height,
    border_radius,
    colors,
    });

// ... (Remaining code)`
this is the code that i have written can i make changes with this code @rickstaa

@razonyang
Copy link
Author

@airwakz Thank you spending time on this, I'm curious how it look likes in the case of multiple rows. i.e. the GitHub example below, the 1 and 3 cards take 1 line for descriptions, and the rest of cards take 2 lines. Would it look like the example (equal height and width for all cards) after fixing this issue?

image

@airwakz
Copy link

airwakz commented Oct 14, 2023

@razonyang yah i kept original 6 in mind before solving this issue as we can select max 6 repos

@rickstaa
Copy link
Collaborator

rickstaa commented Oct 14, 2023

`/ Import the card_width logic from the "stats card" import { CARD_MIN_WIDTH as REPO_CARD_MIN_WIDTH, CARD_DEFAULT_WIDTH as REPO_CARD_DEFAULT_WIDTH } from "./stats-card.js";

// ... (Existing code)

// Define your own minCardWidth and defaultCardWidth for the "repo card" const REPO_CARD_ICON_SIZE = 16; // Adjust this value as needed const REPO_CARD_MIN_WIDTH = /* Calculate the minimum width for the repo card /; const REPO_CARD_DEFAULT_WIDTH = / Calculate the default width for the repo card */;

/**

  • Check the card_width option and set the card width accordingly.
    */
    const cardWidth = options.card_width
    ? isNaN(options.card_width)
    ? REPO_CARD_DEFAULT_WIDTH
    : Math.max(REPO_CARD_MIN_WIDTH, options.card_width) // Ensure it doesn't go below the minimum width
    : REPO_CARD_DEFAULT_WIDTH;

// ... (Existing code)

/**

  • Create the Card instance with the calculated cardWidth.
    */
    const card = new Card({
    defaultTitle: header.length > 35 ? ${header.slice(0, 35)}... : header,
    titlePrefixIcon: icons.contribs,
    width: cardWidth, // Use the calculated card width
    height,
    border_radius,
    colors,
    });

// ... (Remaining code)` this is the code that i have written can i make changes with this code @rickstaa

@airwakz Can you maybe create a pull request? This will make it a lot easier for me to review your code.

@rickstaa
Copy link
Collaborator

@airwakz Thank you spending time on this, I'm curious how it look likes in the case of multiple rows. i.e. the GitHub example below, the 1 and 3 cards take 1 line for descriptions, and the rest of cards take 2 lines. Would it look like the example (equal height and width for all cards) after fixing this issue?

image

I think the width and height should be handled by the user through the card_width and card_height arguments. This allows people to create the kind of grid they want.

@airwakz
Copy link

airwakz commented Oct 14, 2023

Now, users can specify the card_width and card_height when using the renderRepoCard function to control the dimensions of the repository card. If not provided, it falls back to default value and can uh tell me its default value
@rickstaa @razonyang

@airwakz
Copy link

airwakz commented Oct 14, 2023

@rickstaa opened a pr just check it out

@airwakz
Copy link

airwakz commented Oct 14, 2023

image

@airwakz
Copy link

airwakz commented Oct 14, 2023

@qwerty541 @rickstaa #3359 i have raised a pull request kindly merge it

rickstaa added a commit to airwakz/github-readme-stats that referenced this issue Oct 16, 2023
Restores the original 'top-languages-card' code since it is not part of
\anuraghazra#2900.
rickstaa added a commit to airwakz/github-readme-stats that referenced this issue Oct 16, 2023
Restores the original 'top-languages-card' code since it is not part of
\anuraghazra#2900.
rickstaa added a commit to airwakz/github-readme-stats that referenced this issue Oct 16, 2023
Restores the original 'top-languages-card' code since it is not part of
\anuraghazra#2900.
@qwerty541 qwerty541 added the repo-card Issues related to the pin/repo card. label Nov 17, 2023
@github-actions github-actions bot added the ⭐ top issue Top issue. label Nov 19, 2023
@qwerty541
Copy link
Collaborator

Pull request #3453 was merged, now equal height for different repositories cards can be achieved by the following way:

Before:

<div align="center"> 
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=rjyo&repo=Air-Test"/>
</a>
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=meako689&repo=FortyTwoTestTask"/>
</a>
</div>

After (&description_lines_count=3):

<div align="center"> 
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=rjyo&repo=Air-Test&description_lines_count=3"/>
</a>
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=meako689&repo=FortyTwoTestTask&description_lines_count=3"/>
</a>
</div>

After (&description_lines_count=1):

<div align="center"> 
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=rjyo&repo=Air-Test&description_lines_count=1"/>
</a>
<a href="https://github.com/anuraghazra/github-readme-stats">
<img src="https://github-readme-stats.vercel.app/api/pin/?username=meako689&repo=FortyTwoTestTask&description_lines_count=1"/>
</a>
</div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. hacktoberfest repo-card Issues related to the pin/repo card. ⭐ top feature Top feature request. ⭐ top issue Top issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants