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

css: disable ligatures in monospace fonts #1851

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

rybak
Copy link
Contributor

@rybak rybak commented Jun 3, 2024

A fix for the readability issue reported in #1817. The commit message basically repeats the bug report.

I am fairly familiar with CSS, but I don't know how exactly .scss works, similar to how I didn't know Ruby in #838 (still don't). However, the things required for this PR were relatively intuitive and I did get the fix working locally simply by guessing. Here are comparisons to the screenshots from the bug report:

what before/after screenshots
"mentions of reflog in monospace" image
"mention of git config" image
"mention of color.diff.new" image

The choice of the font in app/assets/stylesheets/variables.scss for the
rule:

	$fixed-width-font-family: Courier, monospace;

can make it harder to read some of the text in the documentation, at
least in some browsers in some OSes. This is especially applicable to
the names of the configuration keys and to mentions of the shell
commands, which people would usually edit in a text editor or in a shell
with a monospace font. Fonts used by <code> tags are also affected.

For example, the following ligatures "meld" the letters together:

  - in config key `gc.reflogExpireUnreachable` the ligature is `fl`
  - in config key `color.diff.new` -- `ff`
  - in command name `git config` -- `fi`

Add `font-variant-ligatures: none;` [1] to all usages of variable
`$fixed-width-font-family` to fix this readability issue.

[1] see documentation of this CSS property on MDN:
    https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-ligatures
Fixes: git#1817
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

On the face of it, I totally agree with the intention of this PR.

I do have to point out that there are people who want ligatures to be used to render their code, see e.g. https://blog.jetbrains.com/idea/2016/06/intellij-idea-2016-2-eap-case-only-renames-in-git-ligatures-background-images-and-more/ (and as always, where there's fans, there's haters, too).

I don't know how exactly .scss works

That's relatively easy to explain (but keep in mind that I am also not an expert): .scss has "front matter" for meta data, and is hence pre-processed into proper .css; Once we move to Hugo, it will be possible to use Hugo-specific constructs to pre-process the style sheets into their final form.

The changes you made look good to me.

@rybak
Copy link
Contributor Author

rybak commented Sep 6, 2024

there are people who want ligatures to be used to render their code, see e.g. https://blog.jetbrains.com/idea/2016/06/intellij-idea-2016-2-eap-case-only-renames-in-git-ligatures-background-images-and-more/

The difference being, that ligatures in JetBrains' font don't change the width of the text. In the blog post it's noticeable on the ligature for != which stretches the not equals sign "≠" to the width of two monospace characters:

image

In case of git docs, the width changes:

image

When a monospace font is monospace for each character equally – it is easier to visually compare the docs with a text editor or a shell/terminal, where the names of these commands and config variables will be read by users.

@dscho
Copy link
Member

dscho commented Sep 6, 2024

That explanation makes sense to me, that the width changes with those ligatures. So yeah, I am in favor of this PR. @pedrorijo91 what's your opinion?

@pedrorijo91
Copy link
Member

i have no strong opinion on fonts tbh 😄

@dscho dscho merged commit f820c53 into git:main Sep 10, 2024
1 check passed
@rybak rybak deleted the monospace-disable-ligatures branch September 11, 2024 23:35
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.

3 participants