fix(pen): resolve ordinary variable references (#622)
* fix(pen): resolve ordinary variable references Recognize nonempty dollar-prefixed variable names while preserving double-dash references and existing bindings. Add parser regressions, a reproducible .pen fixture, and visual coverage comparing variable references with literal values. Update the Unreleased changelog for #563. * test(pen): strengthen variable reference coverage --------- Co-authored-by: Danila Poyarkov <dev@dannote.net>
This commit is contained in:
parent
076f7ce0d9
commit
c946337f3d
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
33
tests/e2e/canvas/pen-variables.spec.ts
Normal file
33
tests/e2e/canvas/pen-variables.spec.ts
Normal file
|
|
@ -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'
|
||||
)
|
||||
})
|
||||
Binary file not shown.
|
After Width: | Height: | Size: 16 KiB |
|
|
@ -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', () => {
|
||||
|
|
|
|||
145
tests/engine/pen/variables.test.ts
Normal file
145
tests/engine/pen/variables.test.ts
Normal file
|
|
@ -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: {} })
|
||||
})
|
||||
})
|
||||
77
tests/fixtures/pen-variables.pen
vendored
Normal file
77
tests/fixtures/pen-variables.pen
vendored
Normal file
|
|
@ -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"
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in a new issue