-
Notifications
You must be signed in to change notification settings - Fork 399
Fix remote fonts not recognised when using a brand extension #13901
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
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
It hallucinated Implementation is messy and IIUC just looks for CSS files in output directory - probably should be refined before merging. |
This is interesting! I think we did use playwright for this kind of CSS verification until now. Mainly to be sure that the CSS applied in browser is the one expected. Or at least this is how I tried to use some CSS value check using playwright. |
src/core/sass/brand.ts
Outdated
| }:${styleString}wght@${weights}&display=${display}');`; | ||
| }; | ||
|
|
||
| const isExternalPath = (path: string) => /^\w+:/.test(path); |
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.
FWIW we have this
Lines 10 to 12 in ba576a8
| export function isHttpUrl(url: string) { | |
| return /^https?:/i.test(url); | |
| } |
but yours is more generic. Also, if you need this one, you should probably add it in one of the core files as we already have several times
quarto-cli/src/core/brand/brand.ts
Lines 275 to 278 in ba576a8
| function isExternalPath(path: string) { | |
| return /^\w+:/.test(path); | |
| } | |
quarto-cli/src/project/types/website/website-navigation.ts
Lines 1543 to 1545 in ba576a8
| function isExternalPath(path: string) { | |
| return /^\w+:/.test(path); | |
| } |
quarto-cli/src/project/types/website/website-utils.ts
Lines 123 to 125 in ba576a8
| function isExternalPath(path: string) { | |
| return /^\w+:/.test(path); | |
| } |
Maybe it is time to have this helper in one place.
Add a new test assertion function that checks regex patterns against CSS files in a document's supporting files directory. This is useful for verifying CSS content that is generated during rendering. Co-Authored-By: Claude Opus 4.5 <[email protected]>
When a brand.yml file with remote font URLs (e.g., https://...) is located in a subfolder (like _extensions/my-brand/), the font URL was incorrectly joined with the path prefix, resulting in broken paths like "_extensions/my-brand/https:/...". Add isExternalPath helper to detect URLs and skip path joining for external font paths. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Move the duplicated isExternalPath function to a single location in src/core/url.ts and update all call sites to import from there. This addresses feedback from @cderv on PR #13901. Co-Authored-By: Claude Opus 4.5 <[email protected]>
fd3eb04 to
bf48448
Compare
Instead of globbing all CSS files in the support directory, parse the HTML document to find `<link rel="stylesheet">` tags and only check those specific CSS files. This is more targeted and accurate. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
Fixes #13685
brand.ymlare incorrectly treated as file paths when the brand file is in a subfolder (like an extension)isExternalPathhelper to detect URLs (paths starting with a protocol likehttps:) and skip path joining for external font pathsensureCssRegexMatchestest assertion for verifying CSS content in supporting filesTest plan
src:url("https://notofonts.github.io/...)_extensions/my-brand/https:)🤖 Generated with Claude Code