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

fix(website): make KaTeX fonts available on alternative locales #7574

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Jun 6, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

The main issue is that the stylesheet is directly linked as a link tag instead of getting bundled, so the url() functions would not have the base URL automatically prepended. If the URL is url(fonts/KaTeX_AMS-Regular.woff2), then on /zh-CN/ it would look for /zh-CN/katex/fonts/KaTeX_AMS-Regular.woff2 which doesn't exist, because static assets aren't duplicated in static/zh-CN. We need to hardcode the full URL.

(I did an initial attempt back in #7572 but made a little mistake by forgetting the /katex/ prefix)

@slorber Do we really want to self-host these assets?

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@Josh-Cena Josh-Cena added the pr: internal This PR does not touch production code, or is not meaningful enough to be in the changelog. label Jun 6, 2022
@Josh-Cena Josh-Cena requested review from slorber and lex111 as code owners June 6, 2022 11:21
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 6, 2022
@netlify
Copy link

netlify bot commented Jun 6, 2022

[V2]

Name Link
🔨 Latest commit 3bbb33f
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/629de32eb51bf300086443fb
😎 Deploy Preview https://deploy-preview-7574--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Jun 6, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 95 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 87 🟢 100 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Jun 6, 2022

Merging for now because en locale is returning 404 as well currently, but we should move back to CDNs.

@Josh-Cena Josh-Cena merged commit 75f0f92 into main Jun 6, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

Size Change: 0 B

Total Size: 801 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.6 kB
website/build/assets/css/styles.********.css 106 kB
website/build/assets/js/main.********.js 603 kB
website/build/index.html 38.9 kB

compressed-size-action

@Josh-Cena Josh-Cena deleted the jc/fix-katex branch June 6, 2022 11:32
@slorber
Copy link
Collaborator

slorber commented Jun 8, 2022

@slorber Do we really want to self-host these assets?

I moved those to self-hosted because I want to be able to work on Docusaurus offline or under very slow network (which I currently have at home btw), and not depend on the resilience/speed of the CDN.

For sure it's possible to disable the fonts in case of need, but that seems appropriate to me to self-host.

@Josh-Cena
Copy link
Collaborator Author

I'm surprised if Docusaurus CDN is more performant than jsdelivr's CDN. For me it's mainly the question of future-proofness. For example, with this change, the CSS is not resilient to site base URL changes.

@slorber
Copy link
Collaborator

slorber commented Jun 15, 2022

I'm surprised if Docusaurus CDN is more performant than jsdelivr's CDN.

This is not a problem of CDN performance. In dev, localhost is always faster than "remotehost" (no matter what it is)

Offline or with a slow network, I should be able to work locally on Docusaurus without pain (ie the first render should not be blocked by many seconds if the request is slow). Using JSDeliver, it was painful to me.

For example, with this change, the CSS is not resilient to site base URL changes.

If you have any idea to make the CSS resilient or use JSDeliver, without breaking the ability to work under a very slow network, I'm fine with your solution.


Maybe a better solution would be to revert this self-hosting PR #6429
And try to use "defer" on the jsdelivr CSS files? I don't think "defer" can be used with stylesheets though 🤷‍♂️

@Josh-Cena
Copy link
Collaborator Author

Maybe a better solution would be to revert this self-hosting PR #6429

That's what I'm asking😅 I'm not sure how gracefully it fails when that CDN asset fails to load—does the site fail to render at all? Or only the math formulae are corrupted? I don't think CSS files are render-blocking—are they?

@slorber
Copy link
Collaborator

slorber commented Jun 15, 2022

That's what I'm asking😅 I'm not sure how gracefully it fails when that CDN asset fails to load—does the site fail to render at all? Or only the math formulae are corrupted? I don't think CSS files are render-blocking—are they?

As far as I remember, the problem was not when it failed to load, but when the request took a very long time. I think it blocks everything on a white screen and the DX only unlocks itself after the requests timeout.

Last time I had network issues, this was impossible to work under these conditions

@Josh-Cena
Copy link
Collaborator Author

Mmm, I see. Let's self-host for now. At least our website isn't deployed anywhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: internal This PR does not touch production code, or is not meaningful enough to be in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants