-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(fonts): relative protocol urls #13765
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
Conversation
🦋 Changeset detectedLatest commit: ebbfd22 The changes in this PR will be included in the next version bump. 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 |
CodSpeed Performance ReportMerging #13765 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a e2e test (or integration test), so we test if the font is correctly loaded? I'm not sure a unit test is enough here
packages/astro/src/assets/fonts/logic/normalize-remote-font-faces.ts
Outdated
Show resolved
Hide resolved
|
I approved this before seeing Ema's request for changes |
|
@ematipico I don't think a e2e test is needed here. The way the fonts are architected allow to test every part in isolation with unit tests. In this case, the bug is not that the font fetcher thinks the url is absolute when it's not (and fails). The bug is that the url it receives is incorrect. So I believe the test is actually fine since it makes sure this kind of url is handled earlier. Let me know what you think! |
Changes
//), which are then considered absolute by the font fetcher and failshttpsTesting
Unit + manual
Docs
Changeset