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

Improve monospaced font on Chrome Android #812

Merged
merged 3 commits into from
Jul 17, 2019
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented May 30, 2019

This improves the monospaced font on Chrome Android.

Problem

It seems that Chrome on Android skips the first 4 fonts from Primer's mono font stack and uses Courier. Which is very thin and probably not meant for smaller text sizes. Interestingly Firefox skips Courier and uses the fallback monospace font.

$mono-font: "SFMono-Regular", Consolas, "Liberation Mono", Menlo, Courier, monospace !default;
Chrome Firefox
chrome firefox

Fix

By switching the order, Chrome now also uses the browser's fallback monospace font before using Courier. This should translate to:

  • Chrome Android: monospace -> Monaco
  • Firefox Android: monospace -> Droid Sans Mono

Alternatives

We could add Monaco and Droid Sans Mono to the font stack. Which would have the same result. But maybe it's ok to leave it up to the browser to pick the "best" monospaced font. In case we don't like it whenever it gets changed, we can override it again.

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented May 30, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

I'm a fan of this. I do wonder which OS/device combos were even using Courier, but I agree that falling back to the OS default is probably better in those cases.

@shawnbot
Copy link
Contributor

@broccolini do you want to weigh in on this before we include it in the next patch release?

@simurai
Copy link
Contributor Author

simurai commented Jun 18, 2019

I slightly changed this PR:

- $mono-font: ... Courier, monospace;
+ $mono-font: ... monospace, Courier;

So now, in the unlikely case (and I would consider it a browser bug) that monospace gets skipped, there is still Courier as a last fallback.

@broccolini would that be ok? It now should be even less risky.

@simurai
Copy link
Contributor Author

simurai commented Jun 18, 2019

After chatting with Shawn, I removed Courier again as a last fallback. It really shouldn't be needed and every browser should support monospace. For example Chrome on macOS uses Courier as the default for monospace.

@simurai simurai mentioned this pull request Jul 2, 2019
16 tasks
@shawnbot shawnbot changed the base branch from master to release-12.5.0 July 17, 2019 19:46
@shawnbot shawnbot merged commit f58773a into release-12.5.0 Jul 17, 2019
@shawnbot shawnbot deleted the mono-font-stack branch July 17, 2019 19:46
sindresorhus added a commit to sindresorhus/modern-normalize that referenced this pull request Dec 23, 2019
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.

2 participants