Skip to content

Commit d40bf65

Browse files
malsomesh9malsomesh9
committed
Address font globals review feedback
1 parent 78bb735 commit d40bf65

16 files changed

Lines changed: 152 additions & 57 deletions

File tree

‎apps/web/client/src/components/store/editor/font/css-theme.ts‎

Lines changed: 25 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { addFontToCssTheme, removeFontFromCssTheme } from '@onlook/fonts';
22
import type { Font, RouterConfig } from '@onlook/models';
3-
import type { EditorEngine } from '../engine';
4-
import type { SandboxManager } from '../sandbox';
5-
import { normalizePath } from '../sandbox/helpers';
3+
import type { EditorEngine } from '@/components/store/editor/engine';
4+
import type { SandboxManager } from '@/components/store/editor/sandbox';
5+
import { normalizePath } from '@/components/store/editor/sandbox/helpers';
66

77
const GLOBAL_CSS_FILE = 'globals.css';
88

@@ -13,10 +13,7 @@ function rankGlobalCssPath(path: string, routerConfig: RouterConfig): number {
1313
return 0;
1414
}
1515

16-
if (
17-
routerConfig.basePath.startsWith('src/') &&
18-
normalizedPath === normalizePath(`src/${GLOBAL_CSS_FILE}`)
19-
) {
16+
if (normalizedPath === normalizePath(`src/${GLOBAL_CSS_FILE}`)) {
2017
return 1;
2118
}
2219

@@ -48,9 +45,10 @@ export const getGlobalCssPath = async (sandbox: SandboxManager): Promise<string
4845
return globalCssFiles[0]?.path ?? null;
4946
};
5047

51-
export const addFontToGlobalCss = async (
52-
font: Font,
48+
const updateGlobalCss = async (
5349
editorEngine: EditorEngine,
50+
transform: (css: string) => string,
51+
errorLabel: string,
5452
): Promise<boolean> => {
5553
try {
5654
const sandbox = editorEngine.activeSandbox;
@@ -64,44 +62,37 @@ export const addFontToGlobalCss = async (
6462
return false;
6563
}
6664

67-
const result = addFontToCssTheme(font, file);
65+
const result = transform(file);
6866
if (result === file) {
6967
return true;
7068
}
7169

7270
await sandbox.writeFile(cssPath, result);
7371
return true;
7472
} catch (error) {
75-
console.error('Error adding font to global CSS:', error);
73+
console.error(errorLabel, error);
7674
return false;
7775
}
7876
};
7977

78+
export const addFontToGlobalCss = async (
79+
font: Font,
80+
editorEngine: EditorEngine,
81+
): Promise<boolean> => {
82+
return updateGlobalCss(
83+
editorEngine,
84+
(css) => addFontToCssTheme(font, css),
85+
'Error adding font to global CSS:',
86+
);
87+
};
88+
8089
export const removeFontFromGlobalCss = async (
8190
fontId: string,
8291
editorEngine: EditorEngine,
8392
): Promise<boolean> => {
84-
try {
85-
const sandbox = editorEngine.activeSandbox;
86-
const cssPath = await getGlobalCssPath(sandbox);
87-
if (!cssPath) {
88-
return false;
89-
}
90-
91-
const file = await sandbox.readFile(cssPath);
92-
if (typeof file !== 'string') {
93-
return false;
94-
}
95-
96-
const result = removeFontFromCssTheme(fontId, file);
97-
if (result === file) {
98-
return true;
99-
}
100-
101-
await sandbox.writeFile(cssPath, result);
102-
return true;
103-
} catch (error) {
104-
console.error('Error removing font from global CSS:', error);
105-
return false;
106-
}
93+
return updateGlobalCss(
94+
editorEngine,
95+
(css) => removeFontFromCssTheme(fontId, css),
96+
'Error removing font from global CSS:',
97+
);
10798
};

‎apps/web/client/src/components/store/editor/font/index.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import { type CodeDiff, type FontUploadFile } from '@onlook/models';
44
import type { Font } from '@onlook/models/assets';
55
import { generate } from '@onlook/parser';
66
import { makeAutoObservable } from 'mobx';
7+
import { addFontToGlobalCss, removeFontFromGlobalCss } from '@/components/store/editor/font/css-theme';
78
import type { EditorEngine } from '../engine';
89
import { addFontToConfig, ensureFontConfigFileExists, getFontConfigPath, readFontConfigFile, removeFontFromConfig, scanExistingFonts, scanFontConfig } from './font-config';
9-
import { addFontToGlobalCss, removeFontFromGlobalCss } from './css-theme';
1010
import { FontSearchManager } from './font-search-manager';
1111
import { uploadFonts } from './font-upload-manager';
1212
import { addFontVariableToRootLayout, clearDefaultFontFromRootLayout, getCurrentDefaultFont, removeFontVariableFromRootLayout, updateDefaultFontInRootLayout, } from './layout-manager';
@@ -105,12 +105,17 @@ export class FontManager {
105105
await this.ensureConfigFilesExist();
106106
const success = await addFontToConfig(font, fontConfigPath, this.editorEngine);
107107
if (success) {
108-
await Promise.all([
108+
const [tailwindUpdated, layoutUpdated, globalCssUpdated] = await Promise.all([
109109
addFontToTailwindConfig(font, this.editorEngine.activeSandbox),
110110
addFontVariableToRootLayout(font.id, this.editorEngine),
111111
addFontToGlobalCss(font, this.editorEngine),
112112
]);
113113

114+
if (!tailwindUpdated || !layoutUpdated || !globalCssUpdated) {
115+
console.error('Failed to apply font to one or more project config files');
116+
return false;
117+
}
118+
114119
// Update the fonts array
115120
this._fonts.push(font);
116121

@@ -145,6 +150,17 @@ export class FontManager {
145150
const result = await removeFontFromConfig(font, fontConfigPath, this.editorEngine);
146151

147152
if (result) {
153+
const [layoutUpdated, tailwindUpdated, globalCssUpdated] = await Promise.all([
154+
removeFontVariableFromRootLayout(font.id, this.editorEngine),
155+
removeFontFromTailwindConfig(font, this.editorEngine.activeSandbox),
156+
removeFontFromGlobalCss(font.id, this.editorEngine),
157+
]);
158+
159+
if (!layoutUpdated || !tailwindUpdated || !globalCssUpdated) {
160+
console.error('Failed to remove font from one or more project config files');
161+
return false;
162+
}
163+
148164
// Remove from fonts array
149165
this._fonts = this._fonts.filter((f) => f.id !== font.id);
150166

@@ -155,13 +171,6 @@ export class FontManager {
155171
this._defaultFont = null;
156172
}
157173

158-
// Remove font variable and font class from layout file
159-
await removeFontVariableFromRootLayout(font.id, this.editorEngine);
160-
161-
// Remove font from Tailwind config
162-
await removeFontFromTailwindConfig(font, this.editorEngine.activeSandbox);
163-
await removeFontFromGlobalCss(font.id, this.editorEngine);
164-
165174
return result;
166175
}
167176
return false;
@@ -358,6 +367,7 @@ export class FontManager {
358367
for (const font of removedFonts) {
359368
await removeFontFromTailwindConfig(font, sandbox);
360369
await removeFontVariableFromRootLayout(font.id, this.editorEngine);
370+
await removeFontFromGlobalCss(font.id, this.editorEngine);
361371
}
362372
}
363373

‎apps/web/client/src/components/store/editor/font/layout-manager.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ export const removeFontVariableFromRootLayout = async (
109109
await editorEngine.activeSandbox.writeFile(layoutPath, newContent);
110110
return true;
111111
}
112-
return false;
112+
return true;
113113
} catch (error) {
114114
console.error(`Error removing font variable`, error);
115115
return false;

‎packages/fonts/src/helpers/css-theme.ts‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { Font } from '@onlook/models';
22

33
const THEME_BLOCK_REGEX = /@theme(?:\s+inline)?\s*\{/g;
44

5-
function findThemeBlock(content: string): { start: number; bodyStart: number; end: number } | null {
5+
function findThemeBlock(content: string): { bodyStart: number; bodyEnd: number } | null {
66
const match = THEME_BLOCK_REGEX.exec(content);
77
THEME_BLOCK_REGEX.lastIndex = 0;
88

@@ -21,9 +21,8 @@ function findThemeBlock(content: string): { start: number; bodyStart: number; en
2121
depth--;
2222
if (depth === 0) {
2323
return {
24-
start: match.index,
2524
bodyStart,
26-
end: index,
25+
bodyEnd: index,
2726
};
2827
}
2928
}
@@ -32,6 +31,10 @@ function findThemeBlock(content: string): { start: number; bodyStart: number; en
3231
return null;
3332
}
3433

34+
function escapeRegExp(value: string): string {
35+
return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
36+
}
37+
3538
function getThemeFontVariableName(fontId: string): string {
3639
return `--font-${fontId}`;
3740
}
@@ -43,30 +46,48 @@ function getFontThemeDeclaration(font: Font): string {
4346
return `${themeVariable}: var(${fontVariable});`;
4447
}
4548

49+
/**
50+
* Adds or updates a Tailwind CSS `@theme` font token for an Onlook-managed font.
51+
*/
4652
export function addFontToCssTheme(font: Font, content: string): string {
4753
const declaration = getFontThemeDeclaration(font);
4854
const themeVariable = getThemeFontVariableName(font.id);
49-
const existingDeclarationRegex = new RegExp(`(^\\s*)${themeVariable}\\s*:[^;]+;`, 'm');
50-
51-
if (existingDeclarationRegex.test(content)) {
52-
return content.replace(existingDeclarationRegex, `$1${declaration}`);
53-
}
5455

5556
const block = findThemeBlock(content);
5657
if (!block) {
5758
const separator = content.endsWith('\n') ? '\n' : '\n\n';
5859
return `${content}${separator}@theme inline {\n ${declaration}\n}\n`;
5960
}
6061

61-
const beforeClose = content.slice(0, block.end).replace(/\s*$/, '');
62-
const afterClose = content.slice(block.end);
62+
const beforeBody = content.slice(0, block.bodyStart);
63+
const body = content.slice(block.bodyStart, block.bodyEnd);
64+
const afterBody = content.slice(block.bodyEnd);
65+
const escapedThemeVariable = escapeRegExp(themeVariable);
66+
const existingDeclarationRegex = new RegExp(`(^\\s*)${escapedThemeVariable}\\s*:[^;]+;`, 'm');
67+
68+
if (existingDeclarationRegex.test(body)) {
69+
return `${beforeBody}${body.replace(existingDeclarationRegex, `$1${declaration}`)}${afterBody}`;
70+
}
6371

64-
return `${beforeClose}\n ${declaration}\n${afterClose}`;
72+
const trimmedBody = body.replace(/\s*$/, '');
73+
return `${beforeBody}${trimmedBody}\n ${declaration}\n${afterBody}`;
6574
}
6675

76+
/**
77+
* Removes a Tailwind CSS `@theme` font token for an Onlook-managed font.
78+
*/
6779
export function removeFontFromCssTheme(fontId: string, content: string): string {
80+
const block = findThemeBlock(content);
81+
if (!block) {
82+
return content;
83+
}
84+
6885
const themeVariable = getThemeFontVariableName(fontId);
69-
const declarationRegex = new RegExp(`\\n?\\s*${themeVariable}\\s*:[^;]+;`, 'g');
86+
const escapedThemeVariable = escapeRegExp(themeVariable);
87+
const declarationRegex = new RegExp(`\\n?\\s*${escapedThemeVariable}\\s*:[^;]+;`, 'g');
88+
const beforeBody = content.slice(0, block.bodyStart);
89+
const body = content.slice(block.bodyStart, block.bodyEnd);
90+
const afterBody = content.slice(block.bodyEnd);
7091

71-
return content.replace(declarationRegex, '');
92+
return `${beforeBody}${body.replace(declarationRegex, '')}${afterBody}`;
7293
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/config.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"font": {
3+
"id": "inter",
4+
"family": "Inter",
5+
"variable": "--font-inter",
6+
"subsets": ["latin"],
7+
"weight": ["400"],
8+
"styles": ["normal"],
9+
"type": "google"
10+
}
11+
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/expected.css‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
:root {
2+
--font-inter: system-ui;
3+
}
4+
5+
@theme inline {
6+
--color-background: #ffffff;
7+
--font-inter: var(--font-inter);
8+
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/input.css‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
:root {
2+
--font-inter: system-ui;
3+
}
4+
5+
@theme inline {
6+
--color-background: #ffffff;
7+
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/config.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"font": {
3+
"id": "font.test",
4+
"family": "Font Test",
5+
"variable": "--font-font-test",
6+
"subsets": ["latin"],
7+
"weight": ["400"],
8+
"styles": ["normal"],
9+
"type": "google"
10+
}
11+
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/expected.css‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
@theme inline {
2+
--font-font-test: var(--font-old);
3+
--font-font.test: var(--font-font-test);
4+
}

‎packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/input.css‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
@theme inline {
2+
--font-font-test: var(--font-old);
3+
--font-font.test: var(--font-old);
4+
}

0 commit comments

Comments
 (0)