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

CommandView: do not change caret size with config.line_height #1080

Merged
merged 1 commit into from Jul 17, 2022

Conversation

jgmdev
Copy link
Member

@jgmdev jgmdev commented Jul 15, 2022

Fixes issue as shown on image

commandview-caret-size

@Guldoman
Copy link
Member

Should we just remove line_height as a config option? Ideally we'd deduce it from the default font size.
Maybe we should introduce something like style.padding.line to allow increasing/decreasing the computed line height.

@TorchedSammy
Copy link
Contributor

i would say no against removing the line height option. i know someone who likes it and i myself think it's a nice thing. padding between lines is different and imo worse to read than just having a different line height

@Guldoman
Copy link
Member

Padding would be in the line. As in not margin.

The end result would be the same, but instead of having a configurable total line height, you'd have a calculated line height + the padding.

@TorchedSammy
Copy link
Contributor

yeah makes sense, but the line height is consistent with smaller fonts while the padding isn't so i would still say to leave it

@jgmdev
Copy link
Member Author

jgmdev commented Jul 17, 2022

config.line_height works pretty well when zooming in and out since it is a ratio proportional to the font size, I guess that a padding could work the same when applying SCALE to it but that is out of scope for this PR, anyways merging this fix.

@jgmdev jgmdev merged commit e4bef5c into lite-xl:master Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants