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

feat: self host api-reference fonts on scalar CDN #1520

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tmastrom
Copy link
Member

@tmastrom tmastrom commented Apr 24, 2024

This PR hosts default api-reference fonts on the scalar CDN. This is currently a drop-in replacement but further optimizations can be made by removing unused alphabets and selectively fetching based on the selected language.

@tmastrom tmastrom requested a review from marclave as a code owner April 24, 2024 19:30
Copy link

changeset-bot bot commented Apr 24, 2024

🦋 Changeset detected

Latest commit: 296bb94

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@scalar/themes Patch
@scalar/fastify-api-reference Patch
@scalar/api-client Patch
@scalar/api-reference Patch
@scalar/play-button Patch
@scalar/api-client-react Patch
@scalar/api-reference-react Patch
@scalar/cli Patch
@scalar/express-api-reference Patch
@scalar/hono-api-reference Patch
@scalar/nestjs-api-reference Patch
@scalar/nextjs-api-reference Patch
@scalar/nuxt Patch
@scalar/docusaurus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

relativeci bot commented Apr 24, 2024

#1589 Bundle Size — 1.92MiB (+0.16%).

296bb94(current) vs 1ce9661 main#1587(baseline)

Warning

Bundle contains 4 duplicate packages – View duplicate packages

Bundle metrics  Change 1 change Regression 1 regression
                 Current
#1589
     Baseline
#1587
Regression  Initial JS 1.92MiB(+0.16%) 1.91MiB
No change  Initial CSS 0B 0B
No change  Cache Invalidation 100% 100%
No change  Chunks 1 1
No change  Assets 1 1
No change  Modules 1083 1083
No change  Duplicate Modules 0 0
No change  Duplicate Code 0% 0%
No change  Packages 158 158
No change  Duplicate Packages 4 4
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#1589
     Baseline
#1587
Regression  JS 1.92MiB (+0.16%) 1.91MiB

Bundle analysis reportBranch tom/host-fonts-on-cdnProject dashboard

Copy link
Member

@hanspagel hanspagel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Any chance we can make them .css files? That’s what they are, also I assume the mimetype will be fixed then (currently text/plain, should be text/css).

Copy link
Member

@hanspagel hanspagel left a comment

Choose a reason for hiding this comment

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

This looks great! Love how clean the CDN URL is now. Just one thing: Both CSS files still point to Google, for example:

https://fonts.gstatic.com/s/jetbrainsmono/v18/tDbY2o-flEEny0FZhsfKu5WU4zr3E_BX0PnT8RD8yKxTN1OVk6OThhvAWV8.woff2

We need to change this to be an URL on our CDN. We don’t want to hit the Google Fonts servers anymore and their URL with the cache hash might change and break in the future.

@tmastrom
Copy link
Member Author

This looks great! Love how clean the CDN URL is now. Just one thing: Both CSS files still point to Google, for example:

https://fonts.gstatic.com/s/jetbrainsmono/v18/tDbY2o-flEEny0FZhsfKu5WU4zr3E_BX0PnT8RD8yKxTN1OVk6OThhvAWV8.woff2

We need to change this to be an URL on our CDN. We don’t want to hit the Google Fonts servers anymore and their URL with the cache hash might change and break in the future.

Good idea. We will now have our own font css files and host the .woff2 font files on the CDN. This should reduce the number of requests necessary 😄

Copy link
Member

@hanspagel hanspagel left a comment

Choose a reason for hiding this comment

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

So clean, so good. Good bye, Google Fonts! :)

marclave
marclave previously approved these changes Apr 29, 2024
@marclave marclave self-requested a review April 29, 2024 17:07
@marclave marclave dismissed their stale review April 29, 2024 17:08

re-reviewing

Copy link
Contributor

@hwkr hwkr left a comment

Choose a reason for hiding this comment

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

Looks like were only hosting and setting up the regular font weight (400) so bold it just being interpolated by the browser (poorly) off of the regular font.

This branch (left) vs main (right)

I've opened a PR (#1576) against this branch to switch us to the variable width version of Inter (which we should do anyway), the CDN font files will need to be updated as well.

@tmastrom tmastrom marked this pull request as draft May 1, 2024 19:53
@hanspagel
Copy link
Member

So great you spotted this! 🙌

@marclave
Copy link
Member

marclave commented May 2, 2024

we are missing some fonts here and weights right @cameronrohani ?

Copy link
Member

@marclave marclave left a comment

Choose a reason for hiding this comment

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

we are missing some weights right @cameronrohani ?

@hwkr
Copy link
Contributor

hwkr commented May 2, 2024

we are missing some weights right @cameronrohani ?

Shouldn't be once we get the variable width font uploaded to the CDN, you'll be able to set any font weight from 100 → 900 (including in between weights like 421 or something), we only use 400, 500 and 600 in our CSS variables but this gives us the option to change in the future or for people themeing to use other font weights.

From Google:

Variable fonts—or, more specifically, OpenType Font Variations—are a relatively new font format introduced in 2016 that allow one font file to contain multiple stylistic variations, and thus break down the strict delineations of “traditional” (static) fonts. Weight, width, style, optical size, etc. can then be manipulated by the designer or adjusted based on contextual rules.
There are three main benefits offered by variable fonts:

  • To compress: the unified delivery of multiple instances—such as a set of different weights—in a single font file that is much smaller than the total file size of a collection of individual font files.
  • To express: the control given to users to select axes values directly, such as a custom weight that is between two named instances of weight. For example, between Regular (400) and Medium (500), a weight of 427 might be just the right one to express a design intent perfectly.
  • To finesse: the control given to programs to select axes values indirectly, based on context. For example, to automatically select an optical size instance using the current point size.

@tmastrom tmastrom changed the title chore: self host api-reference fonts on scalar CDN feat: self host api-reference fonts on scalar CDN May 2, 2024
@tmastrom tmastrom force-pushed the tom/host-fonts-on-cdn branch 2 times, most recently from 6ebe762 to b697e07 Compare May 6, 2024 19:31
@tmastrom tmastrom marked this pull request as ready for review May 6, 2024 19:31
@tmastrom tmastrom requested a review from marclave May 6, 2024 19:31
@hanspagel
Copy link
Member

I think this is ready to be merged, needs your final approvement @marclave. 🙏 Let me know if I can further help with that.

Copy link
Contributor

Copy link
Member

@marclave marclave left a comment

Choose a reason for hiding this comment

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

tested! let's go!

Copy link
Member

@marclave marclave left a comment

Choose a reason for hiding this comment

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

wait just thinking if we can add a flag to quickly add google fonts 🤔

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.

None yet

5 participants