diff --git a/CHANGELOG.md b/CHANGELOG.md index 982c89c17..31a51cbfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ ### Fixed +- Resolve `$name` references in imported `.pen` fills, stroke fills, font families, dimensions, and spacing without requiring a `--` prefix. (#563) - Prevent the stock photo tool from replacing text, lines, structural layers, or containers with content while supporting closed shape geometry. - Preserve explicit text alignment metadata on imported Figma vectors across save and reload. diff --git a/packages/pen/src/convert.ts b/packages/pen/src/convert.ts index 137b1f90e..79b9a18d5 100644 --- a/packages/pen/src/convert.ts +++ b/packages/pen/src/convert.ts @@ -145,7 +145,7 @@ function defaultForType(type: VariableType): VariableValue { } export function isVarRef(val: unknown): val is string { - return typeof val === 'string' && val.startsWith('$--') + return typeof val === 'string' && val.startsWith('$') && val.length > 1 } function varName(ref: string): string { diff --git a/tests/e2e/canvas/pen-variables.spec.ts b/tests/e2e/canvas/pen-variables.spec.ts new file mode 100644 index 000000000..25456b2fa --- /dev/null +++ b/tests/e2e/canvas/pen-variables.spec.ts @@ -0,0 +1,33 @@ +import { expect, test, useEditorSetup } from '#tests/e2e/fixtures' + +const editor = useEditorSetup('/?test&no-chrome&no-rulers') + +test('renders imported .pen variable colors and fonts like literal values', async () => { + await editor.page.evaluate(() => + window.openPencil?.openFile?.('/tests/fixtures/pen-variables.pen') + ) + await editor.canvas.waitForInit() + expect( + await editor.page.evaluate(() => { + const store = window.openPencil?.getStore?.() + return ['literal-text', 'plain-text', 'dashed-text'].map( + (id) => store?.graph.getNode(id)?.fontFamily + ) + }) + ).toEqual(['Noto Naskh Arabic', 'Noto Naskh Arabic', 'Noto Naskh Arabic']) + await editor.page.evaluate(() => { + const store = window.openPencil?.getStore?.() + if (!store) throw new Error('OpenPencil store not initialized') + store.state.zoom = 1 + store.state.panX = 0 + store.state.panY = 0 + store.clearSelection() + store.requestRender() + }) + await editor.canvas.waitForRender() + editor.canvas.assertNoErrors() + + expect(await editor.canvas.screenshotCanvasRegion(860, 300)).toMatchSnapshot( + 'pen-variable-colors-and-fonts.png' + ) +}) diff --git a/tests/e2e/canvas/pen-variables.spec.ts-snapshots/pen-variable-colors-and-fonts-openpencil-darwin.png b/tests/e2e/canvas/pen-variables.spec.ts-snapshots/pen-variable-colors-and-fonts-openpencil-darwin.png new file mode 100644 index 000000000..78ac570bf Binary files /dev/null and b/tests/e2e/canvas/pen-variables.spec.ts-snapshots/pen-variable-colors-and-fonts-openpencil-darwin.png differ diff --git a/tests/engine/pen/var-padding.test.ts b/tests/engine/pen/var-padding.test.ts index df412835c..52a19acb2 100644 --- a/tests/engine/pen/var-padding.test.ts +++ b/tests/engine/pen/var-padding.test.ts @@ -121,6 +121,13 @@ describe('applyPadding — variable resolution (#201)', () => { }) describe('isVarRef', () => { + test.each(['$merk-blauw', '$font-tekst', '$color.background', '$x'])( + 'recognizes %s as a variable reference', + (reference) => { + expect(isVarRef(reference)).toBe(true) + } + ) + test('recognizes $--xxx as variable reference', () => { expect(isVarRef('$--spacing-lg')).toBe(true) expect(isVarRef('$--color-primary')).toBe(true) @@ -130,6 +137,8 @@ describe('isVarRef', () => { expect(isVarRef('24')).toBe(false) expect(isVarRef('spacing-lg')).toBe(false) expect(isVarRef('')).toBe(false) + expect(isVarRef('$')).toBe(false) + expect(isVarRef('price$')).toBe(false) }) test('rejects non-string values', () => { diff --git a/tests/engine/pen/variables.test.ts b/tests/engine/pen/variables.test.ts new file mode 100644 index 000000000..965f9da36 --- /dev/null +++ b/tests/engine/pen/variables.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, test } from 'bun:test' + +import { parsePenFile, type PenDocument, type PenNode } from '@open-pencil/pen' + +const BLUE = { r: 64 / 255, g: 148 / 255, b: 208 / 255, a: 1 } + +function parseVariableDocument(prefix: string, children: PenNode[]) { + const document: PenDocument = { + version: '2.14', + variables: { + [`${prefix}merk-blauw`]: { type: 'color', value: '#4094D0' }, + [`${prefix}font-tekst`]: { type: 'string', value: 'Barlow' }, + [`${prefix}spacing`]: { type: 'number', value: 24 }, + [`${prefix}size`]: { type: 'number', value: 240 } + }, + children + } + return parsePenFile(JSON.stringify(document)) +} + +test('keeps plain and double-dash variable names distinct in one document', () => { + const document: PenDocument = { + version: '2.14', + variables: { + blue: { type: 'color', value: '#4094D0' }, + '--blue': { type: 'color', value: '#FFFFFF' } + }, + children: [ + { id: 'plain', type: 'frame', fill: '$blue' }, + { id: 'dashed', type: 'frame', fill: '$--blue' } + ] + } + const graph = parsePenFile(JSON.stringify(document)) + const plain = graph.getNode('plain') + const dashed = graph.getNode('dashed') + const plainId = plain?.boundVariables['fills[0]'] + const dashedId = dashed?.boundVariables['fills[0]'] + + expect(plain?.fills[0]?.color).toEqual(BLUE) + expect(dashed?.fills[0]?.color).toEqual({ r: 1, g: 1, b: 1, a: 1 }) + expect(plainId).toBeDefined() + expect(dashedId).toBeDefined() + expect(plainId).not.toBe(dashedId) + expect(graph.variables.get(plainId ?? '')?.name).toBe('blue') + expect(graph.variables.get(dashedId ?? '')?.name).toBe('--blue') +}) + +describe.each(['', '--'])('parsePenFile — $%sname references (#563)', (prefix) => { + const colorRef = `$${prefix}merk-blauw` + const fontRef = `$${prefix}font-tekst` + const spacingRef = `$${prefix}spacing` + const sizeRef = `$${prefix}size` + + test.each([ + ['string', colorRef], + ['object', { type: 'color', color: colorRef }], + ['array', [{ type: 'color', color: colorRef }]] + ] satisfies [string, PenNode['fill']][])( + 'resolves %s fills and preserves color bindings', + (_name, fill) => { + const graph = parseVariableDocument(prefix, [ + { id: 'literal', type: 'frame', fill: '#4094D0', width: 240, height: 120 }, + { + id: 'bound', + type: 'frame', + fill, + stroke: { align: 'inside', thickness: 2, fill: colorRef }, + width: 240, + height: 120 + } + ]) + const node = graph.getNode('bound') + const variable = [...graph.variables.values()].find( + (candidate) => candidate.name === `${prefix}merk-blauw` + ) + + expect(variable).toBeDefined() + expect(node?.fills).toEqual(graph.getNode('literal')?.fills) + expect(node?.fills[0]?.color).toEqual(BLUE) + expect(node?.strokes[0]?.color).toEqual(BLUE) + expect(node?.boundVariables).toEqual({ + 'fills[0]': variable?.id, + 'strokes[0]': variable?.id + }) + expect(graph.getNode('literal')?.boundVariables).toEqual({}) + } + ) + + test('resolves and binds font families without expanding text content', () => { + const graph = parseVariableDocument(prefix, [ + { id: 'literal', type: 'text', content: colorRef, fontFamily: 'Barlow' }, + { id: 'bound', type: 'text', content: colorRef, fontFamily: fontRef } + ]) + const node = graph.getNode('bound') + const fontId = node?.boundVariables.fontFamily + + expect(node?.fontFamily).toBe('Barlow') + expect(node?.fontFamily).toBe(graph.getNode('literal')?.fontFamily) + expect(node?.text).toBe(colorRef) + expect(fontId).toBeDefined() + expect(graph.variables.get(fontId ?? '')?.name).toBe(`${prefix}font-tekst`) + expect(graph.getNode('literal')?.boundVariables).toEqual({}) + }) + + test('resolves numeric references in sizing, spacing, padding, and corners', () => { + const graph = parseVariableDocument(prefix, [ + { + id: 'frame', + type: 'frame', + layout: 'horizontal', + width: sizeRef, + height: sizeRef, + gap: spacingRef, + padding: spacingRef, + cornerRadius: spacingRef + } + ]) + + expect(graph.getNode('frame')).toMatchObject({ + width: 240, + height: 240, + itemSpacing: 24, + paddingTop: 24, + paddingRight: 24, + paddingBottom: 24, + paddingLeft: 24, + cornerRadius: 24 + }) + }) + + test('uses existing fallbacks without binding unknown variables', () => { + const unknown = `$${prefix}unknown` + const graph = parseVariableDocument(prefix, [ + { id: 'frame', type: 'frame', fill: unknown, width: unknown }, + { id: 'text', type: 'text', fontFamily: unknown } + ]) + + expect(graph.getNode('frame')).toMatchObject({ + fills: [{ color: { r: 0, g: 0, b: 0, a: 1 } }], + width: 0, + boundVariables: {} + }) + expect(graph.getNode('text')).toMatchObject({ fontFamily: '', boundVariables: {} }) + }) +}) diff --git a/tests/fixtures/pen-variables.pen b/tests/fixtures/pen-variables.pen new file mode 100644 index 000000000..2bc557ab3 --- /dev/null +++ b/tests/fixtures/pen-variables.pen @@ -0,0 +1,77 @@ +{ + "version": "2.14", + "variables": { + "merk-blauw": { "type": "color", "value": "#4094D0" }, + "font-tekst": { "type": "string", "value": "Noto Naskh Arabic" }, + "--merk-blauw": { "type": "color", "value": "#4094D0" }, + "--font-tekst": { "type": "string", "value": "Noto Naskh Arabic" } + }, + "children": [ + { + "id": "literal", + "type": "frame", + "name": "Literal values", + "x": 60, + "y": 80, + "width": 220, + "height": 160, + "fill": "#4094D0", + "children": [ + { + "id": "literal-text", + "type": "text", + "x": 24, + "y": 60, + "content": "مرحبا بالعالم", + "fontFamily": "Noto Naskh Arabic", + "fontSize": 22, + "fill": "#FFFFFF" + } + ] + }, + { + "id": "plain", + "type": "frame", + "name": "$name references", + "x": 320, + "y": 80, + "width": 220, + "height": 160, + "fill": "$merk-blauw", + "children": [ + { + "id": "plain-text", + "type": "text", + "x": 24, + "y": 60, + "content": "مرحبا بالعالم", + "fontFamily": "$font-tekst", + "fontSize": 22, + "fill": "#FFFFFF" + } + ] + }, + { + "id": "dashed", + "type": "frame", + "name": "$--name references", + "x": 580, + "y": 80, + "width": 220, + "height": 160, + "fill": "$--merk-blauw", + "children": [ + { + "id": "dashed-text", + "type": "text", + "x": 24, + "y": 60, + "content": "مرحبا بالعالم", + "fontFamily": "$--font-tekst", + "fontSize": 22, + "fill": "#FFFFFF" + } + ] + } + ] +}