Skip to content

Vendor FontAwesome and Pure-CSS #981

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

Merged
merged 5 commits into from
Aug 21, 2020

Conversation

Kixiron
Copy link
Member

@Kixiron Kixiron commented Aug 16, 2020

Vendor all of our CSS, allows more users to access our site while simultaneously making less requests to load our page

Since we're using SASS for everything, this also allows for us to serve one single CSS file instead of the previous 4+

Page Current Requests PR Requests
Home page 14 11
Rustdoc page 29 19

Closes #979 and #921.

@jyn514 jyn514 added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Aug 16, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Where did you download the static files from? Can you give some commands that can replicate the changes to vendor?

@jyn514
Copy link
Member

jyn514 commented Aug 16, 2020

You also need to update the dockerfile.

--- stderr
thread 'main' panicked at 'Error compiling sass: Error: File to import not found or unreadable: fontawesome.
        on line 2:1 of templates/style/base.scss
>> @import "vars", "rustdoc", "utils", "navbar", "fontawesome", "regular", "sol
   ^

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Got some time to review this today :)

// Variables
// --------------------------

$fa-font-path: "../-/static" !default;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing the path in the vendored file (which would override it when we update FA), could you set the variable in our own base.scss?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the recommendation in fontawesome's docs, but I can do whatever

Copy link
Member

Choose a reason for hiding this comment

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

Why are they recommending that :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea to be perfectly honest

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this will be fixed whenever we can use scss modules, at that time we can use something along these lines

@use "fontawesome" with (
    $fa-font-path: "../-/static"
);
// For more info see https://sass-lang.com/documentation/at-rules/use#configuration

Modules are nice for other reasons as well, they allow namespacing and explicit access to external variables, mixins and rules

@pietroalbini
Copy link
Member

If the goal is getting docs.rs up and running in China as soon as possible, I'm happy to not block on my comments and merge, if a followup PR is made addressing them.

@Kixiron
Copy link
Member Author

Kixiron commented Aug 19, 2020

I'm so sorry, I've been taking a breather the last few days but I'll get right on this

Kixiron and others added 3 commits August 19, 2020 12:43
Comment on lines +52 to +53
// MimeGuess misses a lot of the file types we need, so there's a small wrapper
// around it
Copy link
Member

Choose a reason for hiding this comment

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

Can we upstream this patch? I don't want to have to special case things if I can at all help it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to guess not since there's lots of open issues & prs for exactly this with no response. It shouldn't really be a huge deal since there's not very many exceptions

Copy link
Member

Choose a reason for hiding this comment

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

@pietroalbini pietroalbini merged commit 1965b14 into rust-lang:master Aug 21, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 21, 2020

Where did you download the static files from? Can you give some commands that can replicate the changes to vendor?

This was never addressed ... it would be great to be able to replicate the changes. Right now it just looks like an opaque 8k line blob.

@jyn514
Copy link
Member

jyn514 commented Aug 21, 2020

This broke fonts and has been reverted in production.

image

downloadable font: download failed (font-family: "Font Awesome 5 Free" style:normal weight:400 stretch:100 src index:1): status=2147746065 source: https://docs.rs/-/-/static/fa-regular-400.woff2

Looks like there was an extra /-/.

@jyn514
Copy link
Member

jyn514 commented Aug 21, 2020

It's because of src:url("../-/static/fa-brands-400.ext").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs.rs does not load in China anymore
3 participants