Skip to content

Commit 0ee1cf3

Browse files
CopilotAarebecca
Copilot
and
Aarebecca
authored
refactor: rename misleading variables and apply targeted code quality improvements (#236)
* Initial plan * refactor: apply code quality improvements across multiple files - Rename `options` state to `previewCode` in Playground.tsx for clarity - Add ITEM_POSITION_H/ITEM_POSITION_V constants in chart-line.tsx - Fix gradient area color to use last colorStopsData color instead of colorPrimary - Add NaN check for width/height in svg.ts getExportViewBox - Add braces around single-statement if in embedIcons - Add empty id guard in collectDefElements - Replace simple regex CSS escape fallback with full CSS.escape algorithm - Remove redundant condition in search.ts loadSearchResource - Use charCodeAt comparison in getCommonPrefixLength for performance - Extract isBetterMatch helper function in templates/utils.ts Agent-Logs-Url: https://github.com/antvis/Infographic/sessions/289607cf-6b2e-4c1c-b79d-ec538a1b098c Co-authored-by: Aarebecca <25787943+Aarebecca@users.noreply.github.com> * fix: move empty id check before visited set, add parentheses for operator precedence - Move !id guard before visited.add() to prevent empty strings being added - Add parentheses around range checks in CSS escape fallback for correct precedence Agent-Logs-Url: https://github.com/antvis/Infographic/sessions/289607cf-6b2e-4c1c-b79d-ec538a1b098c Co-authored-by: Aarebecca <25787943+Aarebecca@users.noreply.github.com> * fix: address review comments in svg export * fix: handle unicode ids in css escape fallback --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Aarebecca <25787943+Aarebecca@users.noreply.github.com> Co-authored-by: Aaron <antvaaron@gmail.com>
1 parent 5a2dab4 commit 0ee1cf3

7 files changed

Lines changed: 208 additions & 18 deletions

File tree

‎__tests__/unit/designs/structures/chart-line.test.tsx‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
/** @jsxImportSource ../../../../src */
2+
import { describe, expect, it } from 'vitest';
3+
import type { ComponentType, ParsedInfographicOptions } from '../../../../src';
4+
import { Rect, renderSVG } from '../../../../src';
5+
import type { BaseItemProps } from '../../../../src/designs/items';
6+
import { ChartLine } from '../../../../src/designs/structures/chart-line';
7+
import type { ParsedData } from '../../../../src/types';
8+
import { minifySvg } from '../../../utils';
9+
10+
const Item: ComponentType<
11+
Omit<BaseItemProps, 'themeColors'> &
12+
Partial<Pick<BaseItemProps, 'themeColors'>>
13+
> = ({ x = 0, y = 0 }) => <Rect x={x} y={y} width={20} height={10} />;
14+
15+
describe('ChartLine', () => {
16+
it('renders a single terminal area stop for the fading gradient', () => {
17+
const data = {
18+
items: [{ value: 10 }, { value: 20 }, { value: 15 }],
19+
xTitle: 'Month',
20+
yTitle: 'Value',
21+
} as ParsedData;
22+
const options = {
23+
data,
24+
themeConfig: {
25+
colorBg: '#ffffff',
26+
colorPrimary: '#1677ff',
27+
},
28+
} as ParsedInfographicOptions;
29+
30+
const svg = minifySvg(
31+
renderSVG(
32+
<ChartLine Item={Item} Items={[]} data={data} options={options} />,
33+
),
34+
);
35+
36+
expect(svg.match(/stop-opacity="0.04"/g) ?? []).toHaveLength(1);
37+
});
38+
});

‎__tests__/unit/exporter/svg.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,58 @@ describe('exporter/svg', () => {
245245
expect(exportedRect?.getAttribute('fill')).not.toContain('url(#');
246246
});
247247

248+
it.each(['1grad', '-1grad', '💡grad'])(
249+
'inlines defs references for ids requiring CSS.escape fallback (%s)',
250+
async (gradientId) => {
251+
const originalCSS = globalThis.CSS;
252+
Object.defineProperty(globalThis, 'CSS', {
253+
configurable: true,
254+
value: undefined,
255+
});
256+
257+
try {
258+
const defs = document.createElementNS(svgNS, 'defs');
259+
const gradient = document.createElementNS(svgNS, 'linearGradient');
260+
gradient.setAttribute('id', gradientId);
261+
const stop1 = document.createElementNS(svgNS, 'stop');
262+
stop1.setAttribute('offset', '0');
263+
stop1.setAttribute('stop-color', '#fff');
264+
const stop2 = document.createElementNS(svgNS, 'stop');
265+
stop2.setAttribute('offset', '1');
266+
stop2.setAttribute('stop-color', '#000');
267+
gradient.appendChild(stop1);
268+
gradient.appendChild(stop2);
269+
defs.appendChild(gradient);
270+
271+
const svg = document.createElementNS(svgNS, 'svg');
272+
svg.setAttribute('viewBox', '0 0 10 10');
273+
svg.appendChild(defs);
274+
275+
const rect = document.createElementNS(svgNS, 'rect');
276+
rect.setAttribute('width', '10');
277+
rect.setAttribute('height', '10');
278+
rect.setAttribute('fill', `url(#${gradientId})`);
279+
svg.appendChild(rect);
280+
281+
const exported = await exportToSVG(svg, { removeIds: true });
282+
283+
const exportedRect = exported.querySelector('rect');
284+
expect(exported.querySelector('defs')).toBeNull();
285+
expect(exportedRect?.getAttribute('fill')).toContain(
286+
'data:image/svg+xml',
287+
);
288+
expect(exportedRect?.getAttribute('fill')).toContain(
289+
`#${encodeURIComponent(gradientId)}`,
290+
);
291+
} finally {
292+
Object.defineProperty(globalThis, 'CSS', {
293+
configurable: true,
294+
value: originalCSS,
295+
});
296+
}
297+
},
298+
);
299+
248300
it('converts foreignObject overflow measurements from client pixels to svg units', async () => {
249301
const svg = document.createElementNS(svgNS, 'svg');
250302
svg.setAttribute('viewBox', '0 0 100 100');

‎dev/src/Playground.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ export const Playground = () => {
3030
const saved = getStoredValues<{ code: string }>(STORAGE_KEY);
3131
return saved?.code || DEFAULT_CODE;
3232
});
33-
const [options, setOptions] = useState(code);
33+
const [previewCode, setPreviewCode] = useState(code);
3434

3535
// Debounce: update preview 500ms after last edit, also persist
3636
useEffect(() => {
3737
const timer = setTimeout(() => {
38-
setOptions(code);
38+
setPreviewCode(code);
3939
setStoredValues(STORAGE_KEY, { code });
4040
}, 500);
4141
return () => clearTimeout(timer);
@@ -86,7 +86,7 @@ export const Playground = () => {
8686

8787
<div style={{ flex: 1, overflow: 'auto' }}>
8888
<Card title="预览" size="small" style={{ height: '100%' }}>
89-
<Infographic options={options} />
89+
<Infographic options={previewCode} />
9090
</Card>
9191
</div>
9292
</div>

‎src/designs/structures/chart-line.tsx‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ export interface ChartLineProps extends BaseStructureProps {
1818
valueFormatter?: (value: number, datum: ItemDatum) => string;
1919
}
2020

21+
const ITEM_POSITION_H = 'center' as const;
22+
const ITEM_POSITION_V = 'normal' as const;
23+
2124
export const ChartLine: ComponentType<ChartLineProps> = (props) => {
2225
const {
2326
Title,
@@ -55,8 +58,8 @@ export const ChartLine: ComponentType<ChartLineProps> = (props) => {
5558
indexes: [0],
5659
datum: items[0],
5760
data,
58-
positionH: 'center',
59-
positionV: 'normal',
61+
positionH: ITEM_POSITION_H,
62+
positionV: ITEM_POSITION_V,
6063
};
6164
const sampleBounds = getElementBounds(<Item {...itemProps} />);
6265
const labelWidth = sampleBounds.width;
@@ -396,7 +399,6 @@ export const ChartLine: ComponentType<ChartLineProps> = (props) => {
396399
</linearGradient>
397400
<linearGradient id={gradientAreaId} x1="0%" y1="0%" x2="100%" y2="0%">
398401
{areaStops}
399-
<stop offset="100%" stopColor={colorPrimary} stopOpacity="0.04" />
400402
</linearGradient>
401403
</Defs>
402404
<Group>{gridElements}</Group>

‎src/exporter/svg.ts‎

Lines changed: 80 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,12 @@ function getExportViewBox(svg: SVGSVGElement) {
2626

2727
const width = parseAbsoluteLength(svg.getAttribute('width'));
2828
const height = parseAbsoluteLength(svg.getAttribute('height'));
29-
if (width > 0 && height > 0) {
29+
if (
30+
!Number.isNaN(width) &&
31+
width > 0 &&
32+
!Number.isNaN(height) &&
33+
height > 0
34+
) {
3035
return { x: 0, y: 0, width, height };
3136
}
3237

@@ -253,7 +258,9 @@ async function embedIcons(svg: SVGSVGElement) {
253258

254259
if (!existsSymbol) {
255260
const symbolElement = document.querySelector(href);
256-
if (symbolElement) defs.appendChild(symbolElement.cloneNode(true));
261+
if (symbolElement) {
262+
defs.appendChild(symbolElement.cloneNode(true));
263+
}
257264
}
258265
});
259266
}
@@ -487,6 +494,7 @@ function collectDefElements(svg: SVGSVGElement, ids: Set<string>) {
487494

488495
while (queue.length) {
489496
const id = queue.shift()!;
497+
if (!id) continue;
490498
if (visited.has(id)) continue;
491499
visited.add(id);
492500

@@ -503,11 +511,80 @@ function collectDefElements(svg: SVGSVGElement, ids: Set<string>) {
503511
return collected;
504512
}
505513

514+
// Fallback implementation based on the CSS.escape algorithm
515+
function cssEscape(value: string): string {
516+
const string = String(value);
517+
const length = string.length;
518+
let result = '';
519+
520+
if (length === 0) {
521+
return '';
522+
}
523+
524+
for (let i = 0; i < length; i++) {
525+
const codeUnit = string.charCodeAt(i);
526+
527+
// Null character
528+
if (codeUnit === 0x0000) {
529+
result += '\uFFFD';
530+
continue;
531+
}
532+
533+
// Control characters or DEL
534+
if ((codeUnit >= 0x0001 && codeUnit <= 0x001f) || codeUnit === 0x007f) {
535+
result += '\\' + codeUnit.toString(16) + ' ';
536+
continue;
537+
}
538+
539+
// Escape if first character is a digit
540+
if (i === 0 && codeUnit >= 0x0030 && codeUnit <= 0x0039) {
541+
result += '\\' + codeUnit.toString(16) + ' ';
542+
continue;
543+
}
544+
545+
// Escape if second character is a digit and first is a hyphen
546+
if (
547+
i === 1 &&
548+
codeUnit >= 0x0030 &&
549+
codeUnit <= 0x0039 &&
550+
string.charCodeAt(0) === 0x002d
551+
) {
552+
result += '\\' + codeUnit.toString(16) + ' ';
553+
continue;
554+
}
555+
556+
// If the character is the first and is a hyphen followed by end of string, escape it
557+
if (i === 0 && length === 1 && codeUnit === 0x002d) {
558+
result += '\\' + string.charAt(i);
559+
continue;
560+
}
561+
562+
// Characters that are safe to use unescaped
563+
if (
564+
codeUnit >= 0x0080 ||
565+
(codeUnit >= 0x0030 && codeUnit <= 0x0039) || // 0-9
566+
(codeUnit >= 0x0041 && codeUnit <= 0x005a) || // A-Z
567+
(codeUnit >= 0x0061 && codeUnit <= 0x007a) || // a-z
568+
codeUnit === 0x002d || // -
569+
codeUnit === 0x005f // _
570+
) {
571+
result += string.charAt(i);
572+
continue;
573+
}
574+
575+
// All other characters
576+
result += '\\' + string.charAt(i);
577+
}
578+
579+
return result;
580+
}
581+
506582
function escapeCssId(id: string) {
507583
if (globalThis.CSS && typeof globalThis.CSS.escape === 'function') {
508584
return globalThis.CSS.escape(id);
509585
}
510-
return id.replace(/([!"#$%&'()*+,./:;<=>?@[\]^`{|}~])/g, '\\$1');
586+
587+
return cssEscape(id);
511588
}
512589

513590
function removeDefs(svg: SVGSVGElement) {

‎src/resource/loaders/search.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,6 @@ export async function loadSearchResource(query: string, format?: string) {
4343
const svgText = commaIndex >= 0 ? result.slice(commaIndex + 1) : result;
4444
return loadSVGResource(svgText);
4545
}
46-
if (mimeType === 'image/svg+xml' && format === 'svg' && isBase64) {
47-
return loadImageBase64Resource(result);
48-
}
4946
return loadImageBase64Resource(result);
5047
}
5148

‎src/templates/utils.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,34 @@ function getCommonPrefixLength(source: string, target: string): number {
4343
const limit = Math.min(source.length, target.length);
4444
let index = 0;
4545

46-
while (index < limit && source[index] === target[index]) {
46+
while (
47+
index < limit &&
48+
source.charCodeAt(index) === target.charCodeAt(index)
49+
) {
4750
index += 1;
4851
}
4952

5053
return index;
5154
}
5255

56+
function isBetterMatch(
57+
bestMatch: string | undefined,
58+
bestDistance: number,
59+
bestPrefixLength: number,
60+
candidateKey: string,
61+
candidateDistance: number,
62+
candidatePrefixLength: number,
63+
): boolean {
64+
return (
65+
candidateDistance < bestDistance ||
66+
(candidateDistance === bestDistance &&
67+
candidatePrefixLength > bestPrefixLength) ||
68+
(candidateDistance === bestDistance &&
69+
candidatePrefixLength === bestPrefixLength &&
70+
(!bestMatch || candidateKey < bestMatch))
71+
);
72+
}
73+
5374
export function findClosestTemplateKey(
5475
type: string,
5576
keys: Iterable<string>,
@@ -71,11 +92,14 @@ export function findClosestTemplateKey(
7192
const prefixLength = getCommonPrefixLength(normalizedType, normalizedKey);
7293

7394
if (
74-
distance < bestDistance ||
75-
(distance === bestDistance && prefixLength > bestPrefixLength) ||
76-
(distance === bestDistance &&
77-
prefixLength === bestPrefixLength &&
78-
(!bestMatch || key < bestMatch))
95+
isBetterMatch(
96+
bestMatch,
97+
bestDistance,
98+
bestPrefixLength,
99+
key,
100+
distance,
101+
prefixLength,
102+
)
79103
) {
80104
bestMatch = key;
81105
bestDistance = distance;

0 commit comments

Comments
 (0)