feat: remove font family hashing in next/font css#53608
feat: remove font family hashing in next/font css#53608ijjk merged 23 commits intovercel:canaryfrom blvdmitry:next-font-fixed-name
Conversation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
|
Not exactly sure why the mdx change is failing exactly since the headings and link approach is exactly the same as for other links in the arguments table. Looking into why this might be happening but would appreciate the help in case you have more context about this. |
|
Fixed the documentation validation |
|
Great work! I suggest checking for |
|
In the discussion someone had issues using the declarations approach but agree that name could be shorter. I'll go through other options again once I'm back from vacation :) Upd: Ah, that was you 😄 I think I remember better now that just redefining a property completely won't work here since it's also responsible for the font fallback so it's not correct to just set it to the fixed name for the whole property value |
|
Excited for this! |
|
Updated to |
|
I assume that in order for all the pipelines to run there should be an approval for them to start. Even if there is no time for the review right now, it would be nice if some one can do that to make sure nothing is failing outside the jobs that have passed 🙌 |
I had issues using the |
|
Has there been any progress on the review process of this PR? Seems there are now some conflicts that must be resolved as well |
|
I can definitely resolve any conflicts but I parked it for now until there is any response from the Next.js team who can enable the rest of the CI. Most of the conflicts in this PR usually happen because fonts metadata gets updated in the main branch so it just requires me to run one script. |
|
Would love to see this PR merged! |
|
It's 2024 now, is there a chance we'll get a review from someone? 😄 |
|
@thomasbennedbaek It's been a long journey so I might not remember all the details correctly but I think fonts already return names with spaces and in the original hashing logic there was a part replacing the spaces with underscores which is now removed. I will check if this true whenever I have time this week |
|
Hey! Reading up on the comments, it's not clear to me if anything is outstanding or if this is ready for review. Happy to toss some dev time behind this if there are outstanding changes preventing review! |
|
@dannyrb Just need to check what's mentioned in #53608 (comment) and get an approval |
|
@blvdmitry I had a look at what Google fonts does by default, which is just |
|
@timneutkens Seems so. I would assume that it's just the generation script for all the exported fonts afterwards that replaces spaces with underscores? Edit: Yep, it does that https://github.com/vercel/next.js/blob/canary/scripts/update-google-fonts.js#L69-L72 @samcx I think it's fine to enable the pipelines then and review the very minimal changes we have now, I've updated the PR title as well |
|
Hey, |
|
@samcx would you have some time next week to check if we need anything else before merging this? I think one of the pipelines was failing last time but I felt like I couldn't figure out the connection back then and with each rebase it has to be manually approved to run again, so will need your help resolving that last bit |
|
Thanks for updating the tests @ijjk, I was just looking through and updating the new once once that appeared since the last ci run and just noticed you've already pushed the changes 🙌 Edit: Seems like some are still failing, not pushing any updates from my side for now to avoid conflicts |
|
Would be great if shipped in the next release ❤️ |
Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
|
Thanks for the PR! |
<!-- Thanks for opening a PR! Your contribution is much appreciated. To make sure your PR is handled as smoothly as possible we request that you follow the checklist sections below. Choose the right checklist for the change(s) that you're making: ## For Contributors ### Improving Documentation - Run `pnpm prettier-fix` to fix formatting issues before opening the PR. - Read the Docs Contribution Guide to ensure your contribution follows the docs guidelines: https://nextjs.org/docs/community/contribution-guide ### Adding or Updating Examples - The "examples guidelines" are followed from our contributing doc https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md - Make sure the linting passes by running `pnpm build && pnpm lint`. See https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md ### Fixing a bug - Related issues linked using `fixes #number` - Tests added. See: https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs - Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md ### Adding a feature - Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. (A discussion must be opened, see https://github.com/vercel/next.js/discussions/new?category=ideas) - Related issues/discussions are linked using `fixes #number` - e2e tests added (https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs - Documentation added - Telemetry added. In case of a feature if it's used or not. - Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md ## For Maintainers - Minimal description (aim for explaining to someone not on the team to understand the PR) - When linking to a Slack thread, you might want to share details of the conclusion - Link both the Linear (Fixes NEXT-xxx) and the GitHub issues - Add review comments if necessary to explain to the reviewer the logic behind a change --> ### What? Adding support for supporting a custom fontFamily name when using next/font ### Why? By default, next/font hashes the font name when generating css to achieve proper scoping. However, that makes it impossible to use next/font with 3rd party libraries that provide CSS with pre-defined font names. ### How? To solve this, I've added a new argument to the next/font function call – `usedFontFamilyName`. It allows developers to pick the fontFamily name that is going to be used in the CSS output instead of the default one and make it work with vendor CSS files. ``` import { Inter } from "next/font/google"; const inter = Inter({ subsets: ["latin"], fixedFontFamily: "Inter", }); ``` Fixes [vercel#43452](vercel#43452) --- Edit: I've changed the implementation to use `disabledFontFamilyHashing` boolean flag which removes the hashing but keeps the original font family name instead of allowing a custom name --------- Co-authored-by: JJ Kasper <jj@jjsweb.site> Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
What?
Adding support for supporting a custom fontFamily name when using next/font
Why?
By default, next/font hashes the font name when generating css to achieve proper scoping.
However, that makes it impossible to use next/font with 3rd party libraries that provide CSS with pre-defined font names.
How?
To solve this, I've added a new argument to the next/font function call –
usedFontFamilyName.It allows developers to pick the fontFamily name that is going to be used in the CSS output instead of the default one and make it work with vendor CSS files.
Fixes #43452
Edit:
I've changed the implementation to use
disabledFontFamilyHashingboolean flag which removes the hashing but keeps the original font family name instead of allowing a custom name