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

feat: use height of text as button height in context menu #254

Closed
wants to merge 2 commits into from

Conversation

casperstorm
Copy link
Member

@casperstorm casperstorm commented Mar 6, 2024

WIP to fix #238

@casperstorm
Copy link
Member Author

@andymandias do you mind testing this if context menus works on linux?

@andymandias
Copy link
Collaborator

Confirmed on IRC but will also confirm here for posterity, it works perfectly. Thank you!

@casperstorm casperstorm requested a review from tarkah March 6, 2024 22:48
Comment on lines +63 to +94
pub family: String,
pub size: f32,
}

impl Default for Font {
fn default() -> Self {
Self {
family: String::from("Iosevka Term"),
size: 13.0
}
}
}

impl Font {
fn deserialize<'de, D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct MaybeFont {
family: Option<String>,
size: Option<f32>,
}

let MaybeFont { family, size } = MaybeFont::deserialize(deserializer)?;
let Font { family: default_family, size: default_size } = Font::default();

Ok(Font {
family: family.unwrap_or(default_family),
size: size.unwrap_or(default_size),
})
}
Copy link
Member

Choose a reason for hiding this comment

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

#[serde(default_with = "default_font_size")]

fn default_font_size() -> f32 {
  13.0
}

Copy link
Member

Choose a reason for hiding this comment

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

Then you can keep default below on Font

Comment on lines +249 to +254
// Height based on font size, with some margin added.
let height = LineHeight::default().to_absolute((font.size + 8.0).into());

button(text(content).style(theme::Text::Primary))
.width(length)
.height(length)
.height(height)
Copy link
Member

Choose a reason for hiding this comment

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

This seems super hacky. Let me play around. We should just be able to use Shrink height and have it work when font size increases....

@tarkah
Copy link
Member

tarkah commented Mar 7, 2024

Supeceded by #255

@tarkah tarkah closed this Mar 7, 2024
@casperstorm casperstorm deleted the bug/238 branch March 12, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rendering Issues When Using Custom Font Family
3 participants