Add card height option #3391
Add card height option #3391
Conversation
|
@DanneelsSophie 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
bot
added
documentation
|
@rickstaa I know this task assigns airwakz, but I share my pull request, I can close but I'm very curious IF you have time to have your opinions in order to improve. (If you have others tasks, I will be very happy to do it thanks 😊 ! |
There was a problem hiding this comment.
@DanneelsSophie, first of all, thanks a lot for your contribution! Your pull request will implement a feature many people are waiting for. I like the PR; it is very well-coded, contains tests and is easy to review ❤️🔥. I made some comments that need to be solved before I can merge this in the main branch, but I already attached the hacktoberfest-accepted label since you meet all the requirements 🎉!
src/cards/gist-card.js
Outdated
| @@ -77,8 +78,9 @@ const renderGistCard = (gistData, options = {}) => { | |||
| .join(""); | |||
|
|
|||
| const lineHeight = descriptionLines > 3 ? 12 : 10; | |||
| const height = | |||
| (descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight; | |||
| const height = card_height | |||
There was a problem hiding this comment.
Here, it would be nice to implement some logic to ensure that the card height is capped at the minimum height needed to display the contents. If I use height=100 the card is messed up 🤔.
There was a problem hiding this comment.
I'm not sure I push a new version do you want the calcul of the min height or you want a fix height :) ?
There was a problem hiding this comment.
@DanneelsSophie, I am afraid the minimum heights need to be calculated dynamically for all cards since the parameters will influence the amount of text on the card. I have not yet investigated the best way to do this.
[](https://github.com/anuraghazra/github-readme-stats)[](https://github.com/anuraghazra/github-readme-stats)There was a problem hiding this comment.
Do you review the new version ? ( I must test this the calculated height it is min height). I would like this code structure is well for you !
There was a problem hiding this comment.
I will review a new version. However, please bear in mind that calculating the minimum required card might be a challenging task. Also, see @qwerty541's comment at #3159 (comment).
There was a problem hiding this comment.
I've kept the calculations from before for the heights. I'll try to read the link you sent me (but it doesn't seem as easy as I thought, I'm on vacation until November 6, butI read the comments :) and your indication to finish this task 😊)
There was a problem hiding this comment.
I've kept the calculations from before for the heights. I'll try to read the link you sent me (but it doesn't seem as easy as I thought, I'm on vacation until November 6, butI read the comments :) and your indication to finish this task 😊)
@DanneelsSophie, yeah, implementing this is quite intense. We discussed this at #3159 (comment). I think we will close the card_height pull requests for now and migrate to using a description_lines_count parameter instead. Sorry for the inconvenience.
There was a problem hiding this comment.
@qwerty541 feel free to close this when you have a description_lines_count pull request 👍🏻.
src/cards/repo-card.js
Outdated
| @@ -87,8 +88,9 @@ 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 | |||
There was a problem hiding this comment.
Here we also need a minimum card_height.
src/cards/stats-card.js
Outdated
| 45 + (statItems.length + 1) * lheight, | ||
| hide_rank ? 0 : statItems.length ? 150 : 180, | ||
| ); | ||
| let height = card_height |
There was a problem hiding this comment.
Here we also need a minimum card height.
src/cards/wakatime-card.js
Outdated
| @@ -259,7 +260,9 @@ const renderWakatimeCard = (stats = {}, options = { hide: [] }) => { | |||
|
|
|||
| // Calculate the card height depending on how many items there are | |||
| // but if rank circle is visible clamp the minimum height to `150` | |||
| let height = Math.max(45 + (filteredLanguages.length + 1) * lheight, 150); | |||
| let height = card_height | |||
There was a problem hiding this comment.
This code needs a minimum as well.
src/cards/top-languages-card.js
Outdated
| @@ -805,7 +806,7 @@ const renderTopLanguages = (topLangs, options = {}) => { | |||
| customTitle: custom_title, | |||
| defaultTitle: i18n.t("langcard.title"), | |||
| width, | |||
| height, | |||
| height: card_height ? card_height : height, | |||
There was a problem hiding this comment.
This card also requires the logic for a minimum card height.
DanneelsSophie
force-pushed
the
feat/add_card_height
branch
from
c5e8610
to
0a5ba28
Compare
There was a problem hiding this comment.
Hey, @DanneelsSophie! Thanks for your efforts opening this pull request, i appreciate your considering to contribute! Our team made a decision to resolve this issue #3159 with an alternate solution, so i have to close this pull request in favour of #3453. @rickstaa already added hacktoberfest-accepted label, so i hope it compensates a little your spended time.
Enregistrement.d.ecran.le.2023-10-18.a.15.40.08.mov
#3387