Skip to content

Monospace fonts are broken again #992

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

Closed
jyn514 opened this issue Aug 22, 2020 · 12 comments · Fixed by #993
Closed

Monospace fonts are broken again #992

jyn514 opened this issue Aug 22, 2020 · 12 comments · Fixed by #993
Labels
A-frontend Area: Web frontend C-bug Category: This is a bug P-medium Medium priority

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 22, 2020

Mostly likely by #981.

https://docs.rs/pulldown-cmark/0.7.2/pulldown_cmark/struct.Parser.html#method.new_with_broken_link_callback
Before: image
Currently (but system-dependent): image

The culprit is style.css, which, now that all CSS is in one file, is not particularly helpful.

code, kbd, pre, samp {
    font-family: monospace,monospace;
    font-size: 1em;
}

cc @Kixiron

@jyn514 jyn514 added C-bug Category: This is a bug A-frontend Area: Web frontend P-medium Medium priority labels Aug 22, 2020
@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

According to #981 (comment), that PR should've been reverted, no? (I can't find the revert actually.)

@jyn514
Copy link
Member Author

jyn514 commented Aug 22, 2020

It was reverted in prod but not in the code. The PR was redeployed after #987.

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

Well something is very broken currently. It seems that style.css contains a redundant copy of normalize.css which is overwriting the custom styles.

image

image

EDIT: Apparently https://github.com/rust-lang/docs.rs/blob/master/vendor/pure-css/css/pure-min.css ships their own normalize.css which is the problem. These vendored css should get included first, followed by rustdoc.css, and finally style.css.

@jyn514
Copy link
Member Author

jyn514 commented Aug 22, 2020

I really wish we hadn't combined our own styles into the rest of the vendored styles 🤦

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

Possible solution approach: Extract vendored css (purecss and fontawesome) into a separate file and use the following css order:

  • normalize.css (from rustdoc)
  • vendored.css (purecss and fontawesome)
  • rustdoc.css
  • light.css + dark.css
  • style.css

EDIT: I could try implementing this, if it's something we'd like to try.

@jyn514
Copy link
Member Author

jyn514 commented Aug 22, 2020

@cynecx are you interested in making a PR for that? The relevant code is https://github.com/rust-lang/docs.rs/blob/master/templates/base.html

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

@jyn514 Sure. I'll try.

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

Oh, one question tho. It seems that purecss already contains a version of normalize.css. Should we just keep that with the one from rustdoc? Or remove the normalize.css from rustdoc?

@jyn514
Copy link
Member Author

jyn514 commented Aug 22, 2020

We can't change rustdoc styles. Ideally we would get rid of the one in purecss, but I don't know how to do that cleanly.

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

@jyn514 We could just remove this line?

<link rel="stylesheet" href="/normalize-{{ rustc_resource_suffix() }}.css" type="text/css" media="all" />

@jyn514
Copy link
Member Author

jyn514 commented Aug 22, 2020

Hmm ... I guess that would work, but then our styles are out of sync with rustdoc if it changes the version of normalize. But if it fixes the issue I'm willing to try it.

@cynecx
Copy link
Contributor

cynecx commented Aug 22, 2020

@jyn514 Well we could just keep both for now and do the removal in a future PR. Does that sound okay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend C-bug Category: This is a bug P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants