-
-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Add card height option #3391
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 height option #3391
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,7 @@ const renderRepoCard = (repo, options = {}) => { | |
show_owner = false, | ||
theme = "default_repocard", | ||
border_radius, | ||
card_height, | ||
border_color, | ||
locale, | ||
} = options; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we also need a minimum card_height. |
||
? card_height | ||
: (descriptionLines > 1 ? 120 : 110) + descriptionLines * lineHeight; | ||
|
||
const i18n = new I18n({ | ||
locale, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,6 +238,7 @@ const renderStatsCard = (stats, options = {}) => { | |
disable_animations = false, | ||
rank_icon = "default", | ||
show = [], | ||
card_height, | ||
} = options; | ||
|
||
const lheight = parseInt(String(line_height), 10); | ||
|
@@ -391,10 +392,12 @@ const renderStatsCard = (stats, options = {}) => { | |
|
||
// 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 + (statItems.length + 1) * lheight, | ||
hide_rank ? 0 : statItems.length ? 150 : 180, | ||
); | ||
let height = card_height | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we also need a minimum card height. |
||
? card_height | ||
: Math.max( | ||
45 + (statItems.length + 1) * lheight, | ||
hide_rank ? 0 : statItems.length ? 150 : 180, | ||
); | ||
|
||
// the lower the user's percentile the better | ||
const progress = 100 - rank.percentile; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -738,6 +738,7 @@ const renderTopLanguages = (topLangs, options = {}) => { | |
border_radius, | ||
border_color, | ||
disable_animations, | ||
card_height, | ||
} = options; | ||
|
||
const i18n = new I18n({ | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This card also requires the logic for a minimum card height. |
||
border_radius, | ||
colors, | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,6 +218,7 @@ const renderWakatimeCard = (stats = {}, options = { hide: [] }) => { | |
langs_count = languages.length, | ||
border_radius, | ||
border_color, | ||
card_height, | ||
} = options; | ||
|
||
const shouldHideLangs = Array.isArray(hide) && hide.length > 0; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code needs a minimum as well. |
||
? card_height | ||
: Math.max(45 + (filteredLanguages.length + 1) * lheight, 150); | ||
|
||
const cssStyles = getStyles({ | ||
titleColor, | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I push a new version do you want the calcul of the min height or you want a fix height :) ?
Uh oh!
There was an error while loading. Please reload this page.
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.
@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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Uh oh!
There was an error while loading. Please reload this page.
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.
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 😊)
Uh oh!
There was an error while loading. Please reload this page.
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.
@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 adescription_lines_count
parameter instead. Sorry for the inconvenience.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.
@qwerty541 feel free to close this when you have a
description_lines_count
pull request 👍🏻.