Skip to content

Commit 555808e

Browse files
jriekenjrieken
authored
Merge pull request microsoft#104644 from annkamsk/dom-api-instead-string-concat
issueReporter: Use DOM API instead of string concatenation
2 parents a2b9a5f + e9495c1 commit 555808e

2 files changed

Lines changed: 138 additions & 97 deletions

File tree

‎src/vs/base/browser/dom.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,6 +1009,18 @@ export function prepend<T extends Node>(parent: HTMLElement, child: T): T {
10091009

10101010
const SELECTOR_REGEX = /([\w\-]+)?(#([\w\-]+))?((\.([\w\-]+))*)/;
10111011

1012+
export function reset<T extends Node>(parent: HTMLElement, ...children: Array<Node | string>) {
1013+
parent.innerText = '';
1014+
coalesce(children)
1015+
.forEach(child => {
1016+
if (child instanceof Node) {
1017+
parent.appendChild(child);
1018+
} else {
1019+
parent.appendChild(document.createTextNode(child as string));
1020+
}
1021+
});
1022+
}
1023+
10121024
export enum Namespace {
10131025
HTML = 'http://www.w3.org/1999/xhtml',
10141026
SVG = 'http://www.w3.org/2000/svg'

‎src/vs/code/electron-sandbox/issue/issueReporterMain.ts‎

Lines changed: 126 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import 'vs/base/browser/ui/codicons/codiconStyles'; // make sure codicon css is
88
import { ElectronService, IElectronService } from 'vs/platform/electron/electron-sandbox/electron';
99
import { ipcRenderer, process } from 'vs/base/parts/sandbox/electron-sandbox/globals';
1010
import { applyZoom, zoomIn, zoomOut } from 'vs/platform/windows/electron-sandbox/window';
11-
import { $, windowOpenNoOpener, addClass } from 'vs/base/browser/dom';
11+
import { $, reset, windowOpenNoOpener, addClass } from 'vs/base/browser/dom';
1212
import { Button } from 'vs/base/browser/ui/button/button';
1313
import { CodiconLabel } from 'vs/base/browser/ui/codicons/codiconLabel';
1414
import * as collections from 'vs/base/common/collections';
@@ -277,33 +277,25 @@ export class IssueReporter extends Disposable {
277277
}
278278

279279
private updateSettingsSearchDetails(data: ISettingsSearchIssueReporterData): void {
280-
const target = document.querySelector('.block-settingsSearchResults .block-info');
280+
const target = document.querySelector<HTMLElement>('.block-settingsSearchResults .block-info');
281281
if (target) {
282-
const details = `
283-
<div class='block-settingsSearchResults-details'>
284-
<div>Query: "${data.query}"</div>
285-
<div>Literal match count: ${data.filterResultCount}</div>
286-
</div>
287-
`;
288-
289-
let table = `
290-
<tr>
291-
<th>Setting</th>
292-
<th>Extension</th>
293-
<th>Score</th>
294-
</tr>`;
295-
296-
data.actualSearchResults
297-
.forEach(setting => {
298-
table += `
299-
<tr>
300-
<td>${setting.key}</td>
301-
<td>${setting.extensionId}</td>
302-
<td>${String(setting.score).slice(0, 5)}</td>
303-
</tr>`;
304-
});
305-
306-
target.innerHTML = `${details}<table>${table}</table>`;
282+
const queryDiv = $<HTMLDivElement>('div', undefined, `Query: "${data.query}"` as string);
283+
const countDiv = $<HTMLElement>('div', undefined, `Literal match count: ${data.filterResultCount}` as string);
284+
const detailsDiv = $<HTMLDivElement>('.block-settingsSearchResults-details', undefined, queryDiv, countDiv);
285+
286+
const table = $('table', undefined,
287+
$('tr', undefined,
288+
$('th', undefined, 'Setting'),
289+
$('th', undefined, 'Extension'),
290+
$('th', undefined, 'Score'),
291+
),
292+
...data.actualSearchResults.map(setting => $('tr', undefined,
293+
$('td', undefined, setting.key),
294+
$('td', undefined, setting.extensionId),
295+
$('td', undefined, String(setting.score).slice(0, 5)),
296+
))
297+
);
298+
reset(target, detailsDiv, table);
307299
}
308300
}
309301

@@ -654,9 +646,9 @@ export class IssueReporter extends Disposable {
654646
issueState.appendChild(issueIcon);
655647
issueState.appendChild(issueStateLabel);
656648

657-
item = $('div.issue', {}, issueState, link);
649+
item = $('div.issue', undefined, issueState, link);
658650
} else {
659-
item = $('div.issue', {}, link);
651+
item = $('div.issue', undefined, link);
660652
}
661653

662654
issues.appendChild(item);
@@ -672,19 +664,19 @@ export class IssueReporter extends Disposable {
672664
}
673665

674666
private setUpTypes(): void {
675-
const makeOption = (issueType: IssueType, description: string) => `<option value="${issueType.valueOf()}">${escape(description)}</option>`;
667+
const makeOption = (issueType: IssueType, description: string) => $('option', { 'value': issueType.valueOf() }, escape(description));
676668

677669
const typeSelect = this.getElementById('issue-type')! as HTMLSelectElement;
678670
const { issueType } = this.issueReporterModel.getData();
679671
if (issueType === IssueType.SettingsSearchIssue) {
680-
typeSelect.innerHTML = makeOption(IssueType.SettingsSearchIssue, localize('settingsSearchIssue', "Settings Search Issue"));
672+
reset(typeSelect, makeOption(IssueType.SettingsSearchIssue, localize('settingsSearchIssue', "Settings Search Issue")));
681673
typeSelect.disabled = true;
682674
} else {
683-
typeSelect.innerHTML = [
675+
reset(typeSelect,
684676
makeOption(IssueType.Bug, localize('bugReporter', "Bug Report")),
685677
makeOption(IssueType.FeatureRequest, localize('featureRequest', "Feature Request")),
686678
makeOption(IssueType.PerformanceIssue, localize('performanceIssue', "Performance Issue"))
687-
].join('\n');
679+
);
688680
}
689681

690682
typeSelect.value = issueType.toString();
@@ -774,9 +766,8 @@ export class IssueReporter extends Disposable {
774766
} else {
775767
show(extensionsBlock);
776768
}
777-
778-
descriptionTitle.innerHTML = `${localize('stepsToReproduce', "Steps to Reproduce")} <span class="required-input">*</span>`;
779-
descriptionSubtitle.innerHTML = localize('bugDescription', "Share the steps needed to reliably reproduce the problem. Please include actual and expected results. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub.");
769+
reset(descriptionTitle, localize('stepsToReproduce', "Steps to Reproduce"), $('span.required-input', undefined, '*'));
770+
reset(descriptionSubtitle, localize('bugDescription', "Share the steps needed to reliably reproduce the problem. Please include actual and expected results. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub."));
780771
} else if (issueType === IssueType.PerformanceIssue) {
781772
show(blockContainer);
782773
show(systemBlock);
@@ -790,11 +781,11 @@ export class IssueReporter extends Disposable {
790781
show(extensionsBlock);
791782
}
792783

793-
descriptionTitle.innerHTML = `${localize('stepsToReproduce', "Steps to Reproduce")} <span class="required-input">*</span>`;
794-
descriptionSubtitle.innerHTML = localize('performanceIssueDesciption', "When did this performance issue happen? Does it occur on startup or after a specific series of actions? We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub.");
784+
reset(descriptionTitle, localize('stepsToReproduce', "Steps to Reproduce"), $('span.required-input', undefined, '*'));
785+
reset(descriptionSubtitle, localize('performanceIssueDesciption', "When did this performance issue happen? Does it occur on startup or after a specific series of actions? We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub."));
795786
} else if (issueType === IssueType.FeatureRequest) {
796-
descriptionTitle.innerHTML = `${localize('description', "Description")} <span class="required-input">*</span>`;
797-
descriptionSubtitle.innerHTML = localize('featureRequestDescription', "Please describe the feature you would like to see. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub.");
787+
reset(descriptionTitle, localize('description', "Description"), $('span.required-input', undefined, '*'));
788+
reset(descriptionSubtitle, localize('featureRequestDescription', "Please describe the feature you would like to see. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub."));
798789
show(problemSource);
799790

800791
if (fileOnExtension) {
@@ -805,8 +796,8 @@ export class IssueReporter extends Disposable {
805796
show(searchedExtensionsBlock);
806797
show(settingsSearchResultsBlock);
807798

808-
descriptionTitle.innerHTML = `${localize('expectedResults', "Expected Results")} <span class="required-input">*</span>`;
809-
descriptionSubtitle.innerHTML = localize('settingsSearchResultsDescription', "Please list the results that you were expecting to see when you searched with this query. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub.");
799+
reset(descriptionTitle, localize('expectedResults', "Expected Results"), $('span.required-input', undefined, '*'));
800+
reset(descriptionSubtitle, localize('settingsSearchResultsDescription', "Please list the results that you were expecting to see when you searched with this query. We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub."));
810801
}
811802
}
812803

@@ -928,42 +919,82 @@ export class IssueReporter extends Disposable {
928919
}
929920

930921
private updateSystemInfo(state: IssueReporterModelData) {
931-
const target = document.querySelector('.block-system .block-info');
922+
const target = document.querySelector<HTMLElement>('.block-system .block-info');
923+
932924
if (target) {
933925
const systemInfo = state.systemInfo!;
934-
let renderedData = `
935-
<table>
936-
<tr><td>CPUs</td><td>${systemInfo.cpus}</td></tr>
937-
<tr><td>GPU Status</td><td>${Object.keys(systemInfo.gpuStatus).map(key => `${key}: ${systemInfo.gpuStatus[key]}`).join('<br>')}</td></tr>
938-
<tr><td>Load (avg)</td><td>${systemInfo.load}</td></tr>
939-
<tr><td>Memory (System)</td><td>${systemInfo.memory}</td></tr>
940-
<tr><td>Process Argv</td><td>${systemInfo.processArgs}</td></tr>
941-
<tr><td>Screen Reader</td><td>${systemInfo.screenReader}</td></tr>
942-
<tr><td>VM</td><td>${systemInfo.vmHint}</td></tr>
943-
</table>`;
926+
const renderedDataTable = $('table', undefined,
927+
$('tr', undefined,
928+
$('td', undefined, 'CPUs'),
929+
$('td', undefined, systemInfo.cpus || ''),
930+
),
931+
$('tr', undefined,
932+
$('td', undefined, 'GPU Status' as string),
933+
$('td', undefined, Object.keys(systemInfo.gpuStatus).map(key => `${key}: ${systemInfo.gpuStatus[key]}`).join('\n')),
934+
),
935+
$('tr', undefined,
936+
$('td', undefined, 'Load (avg)' as string),
937+
$('td', undefined, systemInfo.load || ''),
938+
),
939+
$('tr', undefined,
940+
$('td', undefined, 'Memory (System)' as string),
941+
$('td', undefined, systemInfo.memory),
942+
),
943+
$('tr', undefined,
944+
$('td', undefined, 'Process Argv' as string),
945+
$('td', undefined, systemInfo.processArgs),
946+
),
947+
$('tr', undefined,
948+
$('td', undefined, 'Screen Reader' as string),
949+
$('td', undefined, systemInfo.screenReader),
950+
),
951+
$('tr', undefined,
952+
$('td', undefined, 'VM'),
953+
$('td', undefined, systemInfo.vmHint),
954+
),
955+
);
956+
reset(target, renderedDataTable);
944957

945958
systemInfo.remoteData.forEach(remote => {
959+
target.appendChild($<HTMLHRElement>('hr'));
946960
if (isRemoteDiagnosticError(remote)) {
947-
renderedData += `
948-
<hr>
949-
<table>
950-
<tr><td>Remote</td><td>${remote.hostName}</td></tr>
951-
<tr><td></td><td>${remote.errorMessage}</td></tr>
952-
</table>`;
961+
const remoteDataTable = $('table', undefined,
962+
$('tr', undefined,
963+
$('td', undefined, 'Remote'),
964+
$('td', undefined, remote.hostName)
965+
),
966+
$('tr', undefined,
967+
$('td', undefined, ''),
968+
$('td', undefined, remote.errorMessage)
969+
)
970+
);
971+
target.appendChild(remoteDataTable);
953972
} else {
954-
renderedData += `
955-
<hr>
956-
<table>
957-
<tr><td>Remote</td><td>${remote.hostName}</td></tr>
958-
<tr><td>OS</td><td>${remote.machineInfo.os}</td></tr>
959-
<tr><td>CPUs</td><td>${remote.machineInfo.cpus}</td></tr>
960-
<tr><td>Memory (System)</td><td>${remote.machineInfo.memory}</td></tr>
961-
<tr><td>VM</td><td>${remote.machineInfo.vmHint}</td></tr>
962-
</table>`;
973+
const remoteDataTable = $('table', undefined,
974+
$('tr', undefined,
975+
$('td', undefined, 'Remote'),
976+
$('td', undefined, remote.hostName)
977+
),
978+
$('tr', undefined,
979+
$('td', undefined, 'OS'),
980+
$('td', undefined, remote.machineInfo.os)
981+
),
982+
$('tr', undefined,
983+
$('td', undefined, 'CPUs'),
984+
$('td', undefined, remote.machineInfo.cpus || '')
985+
),
986+
$('tr', undefined,
987+
$('td', undefined, 'Memory (System)' as string),
988+
$('td', undefined, remote.machineInfo.memory)
989+
),
990+
$('tr', undefined,
991+
$('td', undefined, 'VM'),
992+
$('td', undefined, remote.machineInfo.vmHint)
993+
),
994+
);
995+
target.appendChild(remoteDataTable);
963996
}
964997
});
965-
966-
target.innerHTML = renderedData;
967998
}
968999
}
9691000

@@ -995,15 +1026,18 @@ export class IssueReporter extends Disposable {
9951026
return 0;
9961027
});
9971028

998-
const makeOption = (extension: IOption, selectedExtension?: IssueReporterExtensionData) => {
1029+
const makeOption = (extension: IOption, selectedExtension?: IssueReporterExtensionData): HTMLOptionElement => {
9991030
const selected = selectedExtension && extension.id === selectedExtension.id;
1000-
return `<option value="${extension.id}" ${selected ? 'selected' : ''}>${escape(extension.name)}</option>`;
1031+
return $<HTMLOptionElement>('option', {
1032+
'value': extension.id,
1033+
'selected': selected || ''
1034+
}, extension.name);
10011035
};
10021036

10031037
const extensionsSelector = this.getElementById('extension-selector');
10041038
if (extensionsSelector) {
10051039
const { selectedExtension } = this.issueReporterModel.getData();
1006-
extensionsSelector.innerHTML = '<option></option>' + extensionOptions.map(extension => makeOption(extension, selectedExtension)).join('\n');
1040+
reset(extensionsSelector, $<HTMLOptionElement>('option'), ...extensionOptions.map(extension => makeOption(extension, selectedExtension)));
10071041

10081042
this.addEventListener('extension-selector', 'change', (e: Event) => {
10091043
const selectedExtensionId = (<HTMLInputElement>e.target).value;
@@ -1071,9 +1105,9 @@ export class IssueReporter extends Disposable {
10711105
}
10721106

10731107
private updateProcessInfo(state: IssueReporterModelData) {
1074-
const target = document.querySelector('.block-process .block-info');
1108+
const target = document.querySelector('.block-process .block-info') as HTMLElement;
10751109
if (target) {
1076-
target.innerHTML = `<code>${state.processInfo}</code>`;
1110+
reset(target, $('code', undefined, state.processInfo));
10771111
}
10781112
}
10791113

@@ -1085,7 +1119,7 @@ export class IssueReporter extends Disposable {
10851119
const target = document.querySelector<HTMLElement>('.block-extensions .block-info');
10861120
if (target) {
10871121
if (this.configuration.disableExtensions) {
1088-
target.innerHTML = localize('disabledExtensions', "Extensions are disabled");
1122+
reset(target, localize('disabledExtensions', "Extensions are disabled"));
10891123
return;
10901124
}
10911125

@@ -1097,8 +1131,7 @@ export class IssueReporter extends Disposable {
10971131
return;
10981132
}
10991133

1100-
const table = this.getExtensionTableHtml(extensions);
1101-
target.innerHTML = `<table>${table}</table>${themeExclusionStr}`;
1134+
reset(target, this.getExtensionTableHtml(extensions), document.createTextNode(themeExclusionStr));
11021135
}
11031136
}
11041137

@@ -1111,28 +1144,24 @@ export class IssueReporter extends Disposable {
11111144
}
11121145

11131146
const table = this.getExtensionTableHtml(extensions);
1114-
target.innerHTML = `<table>${table}</table>`;
1147+
target.innerText = '';
1148+
target.appendChild(table);
11151149
}
11161150
}
11171151

1118-
private getExtensionTableHtml(extensions: IssueReporterExtensionData[]): string {
1119-
let table = `
1120-
<tr>
1121-
<th>Extension</th>
1122-
<th>Author (truncated)</th>
1123-
<th>Version</th>
1124-
</tr>`;
1125-
1126-
table += extensions.map(extension => {
1127-
return `
1128-
<tr>
1129-
<td>${extension.name}</td>
1130-
<td>${extension.publisher.substr(0, 3)}</td>
1131-
<td>${extension.version}</td>
1132-
</tr>`;
1133-
}).join('');
1134-
1135-
return table;
1152+
private getExtensionTableHtml(extensions: IssueReporterExtensionData[]): HTMLTableElement {
1153+
return $('table', undefined,
1154+
$('tr', undefined,
1155+
$('th', undefined, 'Extension'),
1156+
$('th', undefined, 'Author (truncated)' as string),
1157+
$('th', undefined, 'Version'),
1158+
),
1159+
...extensions.map(extension => $('tr', undefined,
1160+
$('td', undefined, extension.name),
1161+
$('td', undefined, extension.publisher.substr(0, 3)),
1162+
$('td', undefined, extension.version),
1163+
))
1164+
);
11361165
}
11371166

11381167
private openLink(event: MouseEvent): void {

0 commit comments

Comments
 (0)