feat: use custom fonts #932
Conversation
benlife5
force-pushed
the
use-custom-fonts
branch
from
09a28d0 to
89c0a62
Compare
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
commit: |
benlife5
force-pushed
the
use-custom-fonts
branch
from
5975199 to
879af2d
Compare
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
WalkthroughThis PR adds structured support for custom fonts and adjusts theme application to accept a relative path. applyTheme now takes a relativePrefixToRoot and composes font link data from separate Google and custom generators. extractInUseFontFamilies returns { inUseGoogleFonts, inUseCustomFonts }. Font injection/loading was refactored (loadFontsIntoDOM, createFontLinkElements) and updateThemeInEditor gained an isThemeMode flag to alter font vs style propagation. Multiple template and editor call sites were updated to pass the new parameters. sequenceDiagram
autonumber
participant Template as Vite/Template (getHeadConfig)
participant Editor as Editor / Internal Components
participant Apply as applyTheme
participant Fonts as visualEditorFonts (generators + loader)
participant Doc as Document / iframe
Template->>Apply: applyTheme(document, relativePrefixToRoot, themeConfig)
Editor->>Apply: applyTheme(streamDocument, relativePrefixToRoot, themeConfig)
Apply->>Fonts: extractInUseFontFamilies(themeData, availableFonts)
Fonts-->>Apply: { inUseGoogleFonts, inUseCustomFonts }
Apply->>Fonts: generateCustomFontLinkData(customFonts, relativePrefixToRoot)
Apply->>Fonts: generateGoogleFontLinkData(inUseGoogleFonts)
Fonts-->>Apply: combined font link elements
Apply->>Doc: updateFontLinksInDocument(document, googleFonts, customFonts)
Apply->>Editor: updateThemeInEditor(newTheme, themeConfig, isThemeMode)
Note over Editor,Doc: updateThemeInEditor may start/stop MutationObserver to sync iframe when not theme-mode
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/internal/components/InternalThemeEditor.tsx (1)
115-123: Remove unreachable condition at line 115.The condition
templateMetadata.isDevMode && !templateMetadata.devOverrideis unreachable. The early return at line 112 exits wheneverlocalDev || templateMetadata.isDevModeis true. By line 115,templateMetadata.isDevModemust be false, making the if block dead code. The else block at line 124 is the only path that executes in this branch. Delete lines 115-123 or clarify if the logic should differ.
🧹 Nitpick comments (2)
packages/visual-editor/src/editor/Editor.tsx (1)
108-118: Comment is slightly outdated.The comment mentions "Load default Google Fonts" but the implementation now also loads custom fonts. Consider updating for clarity.
- // Load default Google Fonts for the font selector dropdown + // Load fonts (Google and custom) for the font selector dropdown in theme mode useEffect(() => {packages/visual-editor/src/utils/applyTheme.test.ts (1)
76-101: Consider adding test coverage for custom fonts.The existing tests verify Google font behavior, but there's no test case for themes using custom fonts. Adding a test that verifies custom font link generation (e.g., a theme with
"'Chillax', sans-serif") would help ensure the new custom font functionality works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (17)
packages/visual-editor/src/components/DefaultThemeConfig.ts(1 hunks)packages/visual-editor/src/components/testing/componentTests.setup.ts(1 hunks)packages/visual-editor/src/editor/Editor.tsx(2 hunks)packages/visual-editor/src/internal/components/InternalThemeEditor.tsx(2 hunks)packages/visual-editor/src/internal/components/LayoutEditor.tsx(1 hunks)packages/visual-editor/src/internal/components/ThemeEditor.tsx(1 hunks)packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx(1 hunks)packages/visual-editor/src/internal/utils/loadMapboxIntoIframe.tsx(3 hunks)packages/visual-editor/src/utils/applyTheme.test.ts(7 hunks)packages/visual-editor/src/utils/applyTheme.ts(5 hunks)packages/visual-editor/src/utils/visualEditorFonts.test.ts(4 hunks)packages/visual-editor/src/utils/visualEditorFonts.ts(7 hunks)packages/visual-editor/src/vite-plugin/templates/directory.tsx(2 hunks)packages/visual-editor/src/vite-plugin/templates/edit.tsx(1 hunks)packages/visual-editor/src/vite-plugin/templates/locator.tsx(2 hunks)packages/visual-editor/src/vite-plugin/templates/main.tsx(2 hunks)starter/src/templates/dev.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
packages/visual-editor/src/internal/components/LayoutEditor.tsx (2)
packages/visual-editor/src/utils/applyTheme.ts (1)
updateThemeInEditor(177-231)packages/visual-editor/src/internal/types/themeData.ts (1)
ThemeData(11-11)
packages/visual-editor/src/vite-plugin/templates/edit.tsx (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(44-103)
packages/visual-editor/src/vite-plugin/templates/directory.tsx (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(44-103)
packages/visual-editor/src/vite-plugin/templates/main.tsx (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(44-103)
packages/visual-editor/src/internal/components/InternalThemeEditor.tsx (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
updateThemeInEditor(177-231)
packages/visual-editor/src/vite-plugin/templates/locator.tsx (3)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(44-103)packages/visual-editor/src/components/DefaultThemeConfig.ts (1)
defaultThemeConfig(370-370)packages/visual-editor/src/components/index.ts (1)
defaultThemeConfig(25-25)
packages/visual-editor/src/components/testing/componentTests.setup.ts (2)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(44-103)packages/visual-editor/src/components/DefaultThemeConfig.ts (1)
defaultThemeConfig(370-370)
packages/visual-editor/src/editor/Editor.tsx (2)
packages/visual-editor/src/utils/visualEditorFonts.ts (2)
loadFontsIntoDOM(162-177)defaultFonts(25-25)packages/visual-editor/src/utils/index.ts (1)
defaultFonts(41-41)
packages/visual-editor/src/utils/visualEditorFonts.ts (2)
packages/visual-editor/src/utils/index.ts (2)
FontRegistry(44-44)defaultFonts(41-41)packages/visual-editor/src/utils/font_registry.js (2)
defaultFonts(3-4567)defaultFonts(3-4567)
packages/visual-editor/src/utils/applyTheme.test.ts (2)
packages/visual-editor/src/utils/applyTheme.ts (2)
applyTheme(44-103)StreamDocument(20-38)packages/visual-editor/src/utils/index.ts (2)
applyTheme(7-7)StreamDocument(7-7)
packages/visual-editor/src/utils/applyTheme.ts (3)
packages/visual-editor/src/utils/visualEditorFonts.ts (5)
generateGoogleFontLinkData(93-108)defaultFonts(25-25)extractInUseFontFamilies(303-337)generateCustomFontLinkData(110-120)FontRegistry(7-7)packages/visual-editor/src/utils/index.ts (3)
defaultFonts(41-41)FontRegistry(44-44)ThemeConfig(37-37)packages/visual-editor/src/internal/utils/internalThemeResolver.ts (1)
generateCssVariablesFromThemeConfig(27-55)
🔇 Additional comments (25)
packages/visual-editor/src/components/DefaultThemeConfig.ts (1)
1-9: Import reordering is non-functional and consistentMoving
constructFontSelectOptionswithin the import block has no behavioral impact and keeps the dependency local to this module; usage below remains unchanged.packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
11-12: Test setup correctly updated for new applyTheme signatureSwitching to local
defaultThemeConfigand callingapplyTheme({}, "./", defaultThemeConfig)matches the new(document, relativePrefixToRoot, themeConfig)signature while preserving the existing behavior of only injecting the<style>block (no fonts) into the test DOM.Also applies to: 20-21
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx (1)
170-176: Theme reset correctly opts into theme mode behaviorAdding the
trueflag toupdateThemeInEditorin the “clear local changes” handler ensures the reset operation uses theme-mode semantics, which matches this component’s role.packages/visual-editor/src/internal/components/ThemeEditor.tsx (1)
207-215: Theme history application correctly uses theme mode flagUpdating
updateThemeInEditorto receive(currentThemeData, themeConfig, true)keeps the effect behavior the same while enabling theme-mode–specific logic in the helper. This is the right place to opt into theme mode globally for the theme editor.packages/visual-editor/src/vite-plugin/templates/directory.tsx (1)
31-35: Head theming correctly wired to relativePrefixToRootUsing
relativePrefixToRootfromTemplateRenderPropswhen callingapplyTheme(document, relativePrefixToRoot, defaultThemeConfig)ensures both Google and custom font links are generated with the correct path prefix for directory pages.Also applies to: 87-91
starter/src/templates/dev.tsx (1)
77-81: Dev template head now aligned with new theming contractPassing
relativePrefixToRootthrough toapplyTheme(document, relativePrefixToRoot, defaultThemeConfig)brings the dev template in line with the runtime templates so font assets (including custom fonts) resolve correctly during local development.Also applies to: 96-100
packages/visual-editor/src/vite-plugin/templates/edit.tsx (1)
41-47: Confirm"./"is the correct prefix for custom font assets on/edit
applyThemeis now invoked asapplyTheme(document, "./", defaultThemeConfig), so any custom font links will be generated relative to./from the/editroute. That’s fine if the bundled font assets are indeed served from/edit/..., but if they live at the site root (e.g./assets/...), this could break font loading in the editor shell.Please sanity-check in the running editor that custom fonts (e.g. Chillax/Clash) load correctly on
/edit; if not, we may need to adjust this prefix (or derive it from the same base config used for other static assets).packages/visual-editor/src/vite-plugin/templates/locator.tsx (1)
32-36: Locator head theming correctly uses relativePrefixToRootThe locator template now passes
relativePrefixToRootintoapplyTheme(document, relativePrefixToRoot, defaultThemeConfig), which keeps font (especially custom font) URLs correct for locator pages regardless of nesting depth.Also applies to: 87-91
packages/visual-editor/src/vite-plugin/templates/main.tsx (1)
35-35: LGTM!The extraction of
relativePrefixToRootfromdataand its usage inapplyThemecorrectly aligns with the updated function signature for custom font asset resolution.Also applies to: 90-90
packages/visual-editor/src/internal/components/LayoutEditor.tsx (1)
112-112: LGTM!Passing
falseforisThemeModecorrectly triggers the in-use font loading behavior for the layout editor, consistent with the PR objective that "the layout editor loads only the in-use fonts."Also applies to: 118-118
packages/visual-editor/src/internal/components/InternalThemeEditor.tsx (1)
110-110: LGTM!Passing
trueforisThemeModecorrectly skips redundant font loading in the theme editor, since fonts are already loaded vialoadFontsIntoDOMin the parentEditor.tsxcomponent.Also applies to: 134-134
packages/visual-editor/src/utils/visualEditorFonts.test.ts (3)
1-1: LGTM!Import updated to use named imports from vitest, consistent with modern vitest usage patterns.
22-27: LGTM!Tests correctly updated to validate the new return shape
{ inUseGoogleFonts, inUseCustomFonts }fromextractInUseFontFamilies. All assertions properly check both Google and custom font categories.Also applies to: 32-35, 43-49, 57-63, 75-81, 100-105
108-129: Good test coverage for custom fonts.The new test case properly validates that fonts not present in the Google fonts registry are categorized as custom fonts, with "Custom Font" appearing in
inUseCustomFontswhile "Open Sans" remains ininUseGoogleFonts.packages/visual-editor/src/internal/utils/loadMapboxIntoIframe.tsx (1)
44-50: LGTM!The font loading is correctly updated to use the new
loadFontsIntoDOMAPI with custom fonts support. The dependency array properly includestemplateMetadata?.customFontsto ensure the effect re-runs when custom fonts change.packages/visual-editor/src/utils/applyTheme.test.ts (1)
13-13: LGTM!Test cases correctly updated to include the new
relativePrefixToRootparameter ("./") in allapplyThemecalls, maintaining alignment with the updated function signature.packages/visual-editor/src/utils/visualEditorFonts.ts (5)
93-108: LGTM!Clean extraction of Google font link generation into a dedicated function with proper type annotation.
110-120: Verify custom font name normalization handles all expected cases.The font name transformation (
replaceAll(" ", "").toLowerCase()) assumes font names only contain alphanumeric characters and spaces. If custom fonts might have hyphens or other special characters (e.g., "My-Font"), those would be preserved in the URL. Ensure this aligns with your y-fonts asset naming convention.
143-159: LGTM!The function correctly combines custom and Google font links, with custom fonts placed first for deterministic ordering. The hardcoded
"./"path is appropriate for the editor iframe context where this function is used.
161-177: LGTM!The function correctly extracts font names from the
customFontsregistry usingObject.keys(). The idempotent check (!document.getElementById) prevents duplicate font loading.
303-336: LGTM!The refactored return type cleanly categorizes fonts into Google and custom buckets. The logic correctly treats any font not found in
availableFonts(the Google fonts registry) as a custom font, aligning with the PR objective.packages/visual-editor/src/utils/applyTheme.ts (4)
44-49: LGTM!The new
relativePrefixToRootparameter enables proper custom font asset resolution across different deployment paths. The signature change is a breaking API change, but per the summary, all callers have been updated accordingly.
66-95: LGTM!The font link generation correctly handles both cases:
- No theme overrides → only default Open Sans (no custom fonts needed)
- Theme overrides present → extracts in-use fonts, generates Google + custom font links
Custom font links are appropriately prepended for deterministic CSS cascade ordering.
158-175: LGTM!The function correctly:
- Removes both Google and custom font links (excluding those marked as visual-editor defaults)
- Only creates new links when there are fonts to load
- Passes both font registries to
createFontLinkElements
177-195: LGTM!The
isThemeModeflag correctly separates concerns:
- Theme mode: Only updates the style tag (fonts loaded separately by theme editor with full font set)
- Layout mode: Updates both style tag and font links (loads only in-use fonts)
This aligns with the PR objective for differentiated font loading behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
215-238: Consider adding timeout and error handling for the MutationObserver.While the observer pattern correctly waits for the iframe, consider these improvements:
- Add a timeout: The observer could run indefinitely if the iframe never appears or loads slowly
- Narrow the observation scope: Using
subtree: trueon the entire document is expensive; consider observing just the iframe container- Add error handling: Check if
iframe.contentDocumentis accessible (CORS restrictions could make it null)Example timeout pattern:
pendingObserver = observer; + + // Add timeout to prevent indefinite observation + const timeoutId = setTimeout(() => { + observer.disconnect(); + pendingObserver = null; + console.warn('Iframe font update timed out after 5s'); + }, 5000); + observer.observe(document, { childList: true, subtree: true, });And add null check:
const pagePreviewStyleTag = iframe?.contentDocument?.getElementById(THEME_STYLE_TAG_ID); if (pagePreviewStyleTag) { + clearTimeout(timeoutId); observer.disconnect(); pendingObserver = null; pagePreviewStyleTag.innerText = newThemeTag; + if (iframe.contentDocument) { updateFontLinksInDocument( iframe.contentDocument!, fontsToLoad, inUseCustomFonts ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/editor/Editor.tsx(2 hunks)packages/visual-editor/src/utils/applyTheme.test.ts(7 hunks)packages/visual-editor/src/utils/applyTheme.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/visual-editor/src/editor/Editor.tsx (1)
packages/visual-editor/src/utils/visualEditorFonts.ts (2)
loadFontsIntoDOM(162-177)defaultFonts(25-25)
packages/visual-editor/src/utils/applyTheme.ts (1)
packages/visual-editor/src/utils/visualEditorFonts.ts (6)
generateGoogleFontLinkData(93-108)defaultFonts(25-25)extractInUseFontFamilies(303-337)generateCustomFontLinkData(110-120)FontRegistry(7-7)createFontLinkElements(143-159)
packages/visual-editor/src/utils/applyTheme.test.ts (2)
packages/visual-editor/src/utils/applyTheme.ts (2)
applyTheme(44-103)StreamDocument(20-38)packages/visual-editor/src/utils/index.ts (2)
applyTheme(7-7)StreamDocument(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
- GitHub Check: create-dev-release
🔇 Additional comments (7)
packages/visual-editor/src/utils/applyTheme.test.ts (2)
13-13: LGTM: Test signature updates are consistent.All test invocations correctly updated to include the new
relativePrefixToRootparameter ("./").Also applies to: 45-45, 70-70, 95-95, 116-116, 127-127, 154-154
38-39: LGTM: Custom font test coverage is thorough.The tests properly verify:
- Custom fonts generate
y-fontslinks with the correct path- Custom font links are prepended before Google font links
- Mixed Google and custom fonts work together
Also applies to: 49-54, 90-90, 97-110
packages/visual-editor/src/editor/Editor.tsx (1)
108-118: LGTM: Font loading logic properly gated by theme mode.The conditional loading ensures:
- Theme editor loads all fonts (Google + custom) for the font picker dropdown
- Layout editor will load only in-use fonts (handled via
updateThemeInEditor)The optional chaining with fallback (
templateMetadata?.customFonts ?? {}) prevents errors if custom fonts are undefined.packages/visual-editor/src/utils/applyTheme.ts (4)
44-95: LGTM: Custom font integration is well-structured.The logic correctly:
- Extracts both Google and custom fonts from theme data
- Generates appropriate link data with the relative path for custom fonts
- Prepends custom font links before Google fonts (ensuring custom fonts are available first)
- Handles the edge case where no Google fonts are found (falls back to Open Sans)
158-175: LGTM: Font link management handles both Google and custom fonts.The function correctly:
- Removes only theme-specific links (preserves
data-visual-editor-fontlinks)- Includes y-fonts links in the removal selector
- Checks total font count before injecting new links
- Passes both font registries to
createFontLinkElements
177-178: LGTM: MutationObserver cleanup pattern addresses previous feedback.The
pendingObserverpattern properly prevents memory leaks by:
- Disconnecting any existing observer before creating a new one (line 186)
- Cleaning up when the iframe is found (lines 223-224)
- Tracking the active observer at module level (line 234)
This resolves the concern raised in previous review comments about accumulating observers.
Also applies to: 186-186, 223-224, 234-234
201-213: LGTM: Theme mode vs layout mode branching is logical.The conditional logic appropriately:
- Skips font loading in theme mode (fonts already loaded via
loadFontsIntoDOMin Editor.tsx)- Loads in-use fonts in layout mode
- Falls back to Open Sans when no Google fonts are found
3 participants
extractInUseFontFamiliesassumes any fonts not in the google list are custom yext fonts and generates a y-fonts linkChillax and Clash are custom fonts:
Screen.Recording.2025-12-05.at.11.25.40.AM.mov