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

Accept editor line height as multiple of font size #125601

Merged
merged 5 commits into from
Jun 11, 2021
Merged

Accept editor line height as multiple of font size #125601

merged 5 commits into from
Jun 11, 2021

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Jun 5, 2021

This PR fixes #115960 with @appetite-j's great suggestion from #105888, also suggested by @usernamehw.
Should also fix #82778.

This is my first PR to VSCode; I'd love your help through!

@ghost
Copy link

ghost commented Jun 5, 2021

CLA assistant check
All CLA requirements met.

@cpsauer
Copy link
Contributor Author

cpsauer commented Jun 5, 2021

Tagging @alexdima, in case he's the right reviewer, since he'd been active (and kind) on the issue, and it looks like a lot of the lines in these files are his :)

@cpsauer
Copy link
Contributor Author

cpsauer commented Jun 5, 2021

Actually, @alexdima, while you're looking at this:
While making this PR, I'd incidentally noticed that the default line height for macOS has been special-cased to 1.5 rather than the 1.35 for other platforms. (See here.)
This seems odd because Xcode (and thus macOS's) default code multiplier is 1.25, I'm pretty sure. Thoughts on removing that special-casing or to change it to 1.25? Asking you because it looks like you last edited those lines, refactoring from elsewhere. Happy to PR if you agree (and not, if opposed).

@alexdima alexdima added this to the June 2021 milestone Jun 8, 2021
@alexdima alexdima enabled auto-merge June 8, 2021 14:53
auto-merge was automatically disabled June 8, 2021 19:57

Head branch was pushed to by a user without write access

@cpsauer
Copy link
Contributor Author

cpsauer commented Jun 8, 2021

@alexdima, thanks for the review and making things better!

Looks like your auto-merge got caught by linter download flakiness, so I took a sec to quickly factor the rounding out to be with the minimum constraint. Hope that's okay, since this would have needed a merge from you anyway.

I'd originally had the integer constraint in there, but I noticed that the lines that follow that section break the integer font size constraint, and it looked like things work just fine with non-integer fonts, at least on macOS. Thoughts?

Also hoping I can still get your thoughts on the macOS golden special-casing, above.

@alexdima
Copy link
Member

Thank you!

@alexdima alexdima merged commit 750390d into microsoft:main Jun 11, 2021
@cpsauer
Copy link
Contributor Author

cpsauer commented Jun 11, 2021

Sweet! Thank you, @alexdima.

@cpsauer
Copy link
Contributor Author

cpsauer commented Jun 11, 2021

Any chance I could still get your take on the integer & macOS golden sub-questions above?

@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to Specify Line Height as a Multiple of Font Size Allow Line Height to be Any number
2 participants