Skip to content

Commit 389e83f

Browse files
Improve <SvgIcon> to make it output svg node and optimize performance (#23570)
Before, the Vue `<SvgIcon>` always outputs DOM nodes like: ```html <span class="outer-class"> <svg class="class-name-defined" ...></svg> </span> ``` The `span` is redundant and I guess such layout and the inconsistent `class/class-name` attributes would cause bugs sooner or later. This PR makes the `<SvgIcon>` clear, and it's faster than before, because it doesn't need to parse the whole SVG string. Before: <details> ![image](https://user-images.githubusercontent.com/2114189/226156474-ce2c57cd-b869-486a-b75b-1eebdac8cdf7.png) </details> After: ![image](https://user-images.githubusercontent.com/2114189/226155774-108f49ed-7512-40c3-94a2-a6e8da18063d.png) --------- Co-authored-by: silverwind <[email protected]>
1 parent 1d35fa0 commit 389e83f

File tree

9 files changed

+77
-24
lines changed

9 files changed

+77
-24
lines changed

templates/projects/view.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@
224224
{{- range index $.LinkedPRs .ID}}
225225
<div class="meta gt-my-2">
226226
<a href="{{$.RepoLink}}/pulls/{{.Index}}">
227-
<span class="gt-m-0 {{if .PullRequest.HasMerged}}purple{{else if .IsClosed}}red{{else}}green{{end}}">{{svg "octicon-git-merge" 16 "gt-mr-2 gt-vm"}}</span>
227+
<span class="gt-m-0 text {{if .PullRequest.HasMerged}}purple{{else if .IsClosed}}red{{else}}green{{end}}">{{svg "octicon-git-merge" 16 "gt-mr-2 gt-vm"}}</span>
228228
<span class="gt-vm">{{.Title}} <span class="text light grey">#{{.Index}}</span></span>
229229
</a>
230230
</div>

templates/repo/projects/view.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@
235235
{{- range index $.LinkedPRs .ID}}
236236
<div class="meta gt-my-2">
237237
<a href="{{$.RepoLink}}/pulls/{{.Index}}">
238-
<span class="gt-m-0 {{if .PullRequest.HasMerged}}purple{{else if .IsClosed}}red{{else}}green{{end}}">{{svg "octicon-git-merge" 16 "gt-mr-2 gt-vm"}}</span>
238+
<span class="gt-m-0 text {{if .PullRequest.HasMerged}}purple{{else if .IsClosed}}red{{else}}green{{end}}">{{svg "octicon-git-merge" 16 "gt-mr-2 gt-vm"}}</span>
239239
<span class="gt-vm">{{.Title}} <span class="text light grey">#{{.Index}}</span></span>
240240
</a>
241241
</div>

templates/user/settings/keys_gpg.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
{{end}}
5555
</div>
5656
<div class="left floated content">
57-
<span class="{{if or .ExpiredUnix.IsZero ($.PageStartTime.Before .ExpiredUnix.AsTime)}}green{{end}}">{{svg "octicon-key" 32}}</span>
57+
<span class="text {{if or .ExpiredUnix.IsZero ($.PageStartTime.Before .ExpiredUnix.AsTime)}}green{{end}}">{{svg "octicon-key" 32}}</span>
5858
</div>
5959
<div class="content">
6060
{{if .Verified}}

templates/user/settings/keys_ssh.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747

4848
</div>
4949
<div class="left floated content">
50-
<span class="tooltip{{if .HasRecentActivity}} green{{end}}" {{if .HasRecentActivity}}data-content="{{$.locale.Tr "settings.key_state_desc"}}"{{end}}>{{svg "octicon-key" 32}}</span>
50+
<span class="tooltip text {{if .HasRecentActivity}}green{{end}}" {{if .HasRecentActivity}}data-content="{{$.locale.Tr "settings.key_state_desc"}}"{{end}}>{{svg "octicon-key" 32}}</span>
5151
</div>
5252
<div class="content">
5353
{{if .Verified}}

web_src/css/base.css

-12
Original file line numberDiff line numberDiff line change
@@ -2420,18 +2420,6 @@ a.ui.basic.label:hover {
24202420
height: 2.1666em !important;
24212421
}
24222422

2423-
span.green .svg {
2424-
color: var(--color-green);
2425-
}
2426-
2427-
span.red .svg {
2428-
color: var(--color-red);
2429-
}
2430-
2431-
span.purple .svg {
2432-
color: var(--color-purple);
2433-
}
2434-
24352423
.migrate .svg.gitea-git {
24362424
color: var(--color-git);
24372425
}

web_src/js/components/ActionRunStatus.vue

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
<template>
2-
<SvgIcon name="octicon-check-circle-fill" class="green" :size="size" :class-name="className" v-if="status === 'success'"/>
2+
<SvgIcon name="octicon-check-circle-fill" class="ui text green" :size="size" :class-name="className" v-if="status === 'success'"/>
33
<SvgIcon name="octicon-skip" class="ui text grey" :size="size" :class-name="className" v-else-if="status === 'skipped'"/>
44
<SvgIcon name="octicon-clock" class="ui text yellow" :size="size" :class-name="className" v-else-if="status === 'waiting'"/>
55
<SvgIcon name="octicon-blocked" class="ui text yellow" :size="size" :class-name="className" v-else-if="status === 'blocked'"/>
66
<SvgIcon name="octicon-meter" class="ui text yellow" :size="size" :class-name="'job-status-rotate ' + className" v-else-if="status === 'running'"/>
7-
<SvgIcon name="octicon-x-circle-fill" class="red" :size="size" v-else/>
7+
<SvgIcon name="octicon-x-circle-fill" class="ui text red" :size="size" v-else/>
88
</template>
99

1010
<script>

web_src/js/components/ContextPopup.vue

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
<div v-if="loading" class="ui active centered inline loader"/>
44
<div v-if="!loading && issue !== null">
55
<p><small>{{ issue.repository.full_name }} on {{ createdAt }}</small></p>
6-
<p><svg-icon :name="icon" :class="[color]" /> <strong>{{ issue.title }}</strong> #{{ issue.number }}</p>
6+
<p><svg-icon :name="icon" :class="['text', color]" /> <strong>{{ issue.title }}</strong> #{{ issue.number }}</p>
77
<p>{{ body }}</p>
88
<div>
99
<div

web_src/js/svg.js

+49-4
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,32 @@ export function svg(name, size = 16, className = '') {
105105

106106
const document = parser.parseFromString(svgs[name], 'image/svg+xml');
107107
const svgNode = document.firstChild;
108-
if (size !== 16) svgNode.setAttribute('width', String(size));
109-
if (size !== 16) svgNode.setAttribute('height', String(size));
110-
// filter array to remove empty string
108+
if (size !== 16) {
109+
svgNode.setAttribute('width', String(size));
110+
svgNode.setAttribute('height', String(size));
111+
}
111112
if (className) svgNode.classList.add(...className.split(/\s+/).filter(Boolean));
112113
return serializer.serializeToString(svgNode);
113114
}
114115

116+
export function svgParseOuterInner(name) {
117+
const svgStr = svgs[name];
118+
if (!svgStr) throw new Error(`Unknown SVG icon: ${name}`);
119+
120+
// parse the SVG string to 2 parts
121+
// * svgInnerHtml: the inner part of the SVG, will be used as the content of the <svg> VNode
122+
// * svgOuter: the outer part of the SVG, including attributes
123+
// the builtin SVG contents are clean, so it's safe to use `indexOf` to split the content:
124+
// eg: <svg outer-attributes>${svgInnerHtml}</svg>
125+
const p1 = svgStr.indexOf('>'), p2 = svgStr.lastIndexOf('<');
126+
if (p1 === -1 || p2 === -1) throw new Error(`Invalid SVG icon: ${name}`);
127+
const svgInnerHtml = svgStr.slice(p1 + 1, p2);
128+
const svgOuterHtml = svgStr.slice(0, p1 + 1) + svgStr.slice(p2);
129+
const svgDoc = parser.parseFromString(svgOuterHtml, 'image/svg+xml');
130+
const svgOuter = svgDoc.firstChild;
131+
return {svgOuter, svgInnerHtml};
132+
}
133+
115134
export const SvgIcon = {
116135
name: 'SvgIcon',
117136
props: {
@@ -120,6 +139,32 @@ export const SvgIcon = {
120139
className: {type: String, default: ''},
121140
},
122141
render() {
123-
return h('span', {innerHTML: svg(this.name, this.size, this.className)});
142+
const {svgOuter, svgInnerHtml} = svgParseOuterInner(this.name);
143+
// https://vuejs.org/guide/extras/render-function.html#creating-vnodes
144+
// the `^` is used for attr, set SVG attributes like 'width', `aria-hidden`, `viewBox`, etc
145+
const attrs = {};
146+
for (const attr of svgOuter.attributes) {
147+
if (attr.name === 'class') continue;
148+
attrs[`^${attr.name}`] = attr.value;
149+
}
150+
attrs[`^width`] = this.size;
151+
attrs[`^height`] = this.size;
152+
153+
// make the <SvgIcon class="foo" class-name="bar"> classes work together
154+
const classes = [];
155+
for (const cls of svgOuter.classList) {
156+
classes.push(cls);
157+
}
158+
// TODO: drop the `className/class-name` prop in the future, only use "class" prop
159+
if (this.className) {
160+
classes.push(...this.className.split(/\s+/).filter(Boolean));
161+
}
162+
163+
// create VNode
164+
return h('svg', {
165+
...attrs,
166+
class: classes,
167+
innerHTML: svgInnerHtml,
168+
});
124169
},
125170
};

web_src/js/svg.test.js

+21-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,28 @@
11
import {expect, test} from 'vitest';
2-
import {svg} from './svg.js';
2+
import {svg, SvgIcon, svgParseOuterInner} from './svg.js';
3+
import {createApp, h} from 'vue';
34

45
test('svg', () => {
56
expect(svg('octicon-repo')).toMatch(/^<svg/);
67
expect(svg('octicon-repo', 16)).toContain('width="16"');
78
expect(svg('octicon-repo', 32)).toContain('width="32"');
89
});
10+
11+
test('svgParseOuterInner', () => {
12+
const {svgOuter, svgInnerHtml} = svgParseOuterInner('octicon-repo');
13+
expect(svgOuter.nodeName).toMatch('svg');
14+
expect(svgOuter.classList.contains('octicon-repo')).toBeTruthy();
15+
expect(svgInnerHtml).toContain('<path');
16+
});
17+
18+
test('SvgIcon', () => {
19+
const root = document.createElement('div');
20+
createApp({render: () => h(SvgIcon, {name: 'octicon-link', size: 24, class: 'base', className: 'extra'})}).mount(root);
21+
const node = root.firstChild;
22+
expect(node.nodeName).toEqual('svg');
23+
expect(node.getAttribute('width')).toEqual('24');
24+
expect(node.getAttribute('height')).toEqual('24');
25+
expect(node.classList.contains('octicon-link')).toBeTruthy();
26+
expect(node.classList.contains('base')).toBeTruthy();
27+
expect(node.classList.contains('extra')).toBeTruthy();
28+
});

0 commit comments

Comments
 (0)