Skip to content

Commit

Permalink
Issue Reporter: UX issues, fixes #42856
Browse files Browse the repository at this point in the history 
* Open issue reporter in same display as workbench

* Styling updates

* Increase url length limit

* normalize extension ids before checking isEnabled
  • Loading branch information
Rachel Macfarlane committed Feb 3, 2018
1 parent 8593a53 commit 5368104
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 42 deletions.
13 changes: 5 additions & 8 deletions src/vs/code/electron-browser/issue/issueReporterMain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ export class IssueReporter extends Disposable {
const descriptionTitle = document.getElementById('issue-description-label');
const descriptionSubtitle = document.getElementById('issue-description-subtitle');


if (issueType === IssueType.Bug) {
show(systemBlock);
hide(processBlock);
Expand All @@ -358,8 +359,7 @@ export class IssueReporter extends Disposable {
show(disabledExtensions);

descriptionTitle.innerHTML = `${localize('stepsToReproduce', "Steps to Reproduce")} <span class="required-input">*</span>`;
show(descriptionSubtitle);
descriptionSubtitle.innerHTML = localize('bugDescription', "How did you encounter this problem? What steps do you need to perform to reliably reproduce the problem? What did you expect to happen and what actually did happen?");
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.");
} else if (issueType === IssueType.PerformanceIssue) {
show(systemBlock);
show(processBlock);
Expand All @@ -368,8 +368,7 @@ export class IssueReporter extends Disposable {
show(disabledExtensions);

descriptionTitle.innerHTML = `${localize('stepsToReproduce', "Steps to Reproduce")} <span class="required-input">*</span>`;
show(descriptionSubtitle);
descriptionSubtitle.innerHTML = localize('performanceIssueDesciption', "When did this performance issue happen? For example, does it occur on startup or after a specific series of actions? Any details you can provide help our investigation.");
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.");
} else {
hide(systemBlock);
hide(processBlock);
Expand All @@ -378,18 +377,16 @@ export class IssueReporter extends Disposable {
hide(disabledExtensions);

descriptionTitle.innerHTML = `${localize('description', "Description")} <span class="required-input">*</span>`;
hide(descriptionSubtitle);
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.");
}
}

private validateInput(inputId: string): boolean {
const inputElement = (<HTMLInputElement>document.getElementById(inputId));
if (!inputElement.value) {
show(document.getElementById(`${inputId}-validation-error`));
inputElement.classList.add('invalid-input');
return false;
} else {
hide(document.getElementById(`${inputId}-validation-error`));
inputElement.classList.remove('invalid-input');
return true;
}
Expand Down Expand Up @@ -438,7 +435,7 @@ export class IssueReporter extends Disposable {
const url = baseUrl + encodeURIComponent(issueBody);

const lengthValidationElement = document.getElementById('url-length-validation-error');
if (url.length > 2081) {
if (url.length > 5400) {
lengthValidationElement.textContent = localize('urlLengthError', "The data exceeds the length limit of 2081. The data is length {0}.", url.length);

This comment has been minimized.

Sign in to view

Copy link
@roblourens

roblourens Feb 5, 2018

Member

@RMacfarlane I noticed that the check is updated but the string is not. You also might want to take the limit out of the localized string, so it doesn't have to be re-localized if the limit changes in the future.

This comment has been minimized.

Sign in to view

Copy link
@roblourens

roblourens Feb 5, 2018

Member

And maybe it should state the units, "5400 characters"
show(lengthValidationElement);
return false;
Expand Down
26 changes: 5 additions & 21 deletions src/vs/code/electron-browser/issue/issueReporterPage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,45 +6,31 @@

import { escape } from 'vs/base/common/strings';
import { localize } from 'vs/nls';
import * as os from 'os';
import pkg from 'vs/platform/node/package';

export default (): string => `
<div id="issue-reporter">
<div id="english" class="input-group hidden">${escape(localize('completeInEnglish', "Please complete the form in English."))}</div>
<div class="section">
<div class="input-group">
<label for="issue-type">${escape(localize('issueTypeLabel', "I want to submit a"))}</label>
<select id="issue-type" class="form-control">
<label id="issue-type-label" class="inline-form-control" for="issue-type">${escape(localize('issueTypeLabel', "This is a"))}</label>
<select id="issue-type" class="inline-form-control">
<option value="0">${escape(localize('bugReporter', "Bug Report"))}</option>
<option value="1">${escape(localize('performanceIssue', "Performance Issue"))}</option>
<option value="2">${escape(localize('featureRequest', "Feature Request"))}</option>
</select>
</div>
<div class="input-group">
<label for="issue-title">${escape(localize('issueTitleLabel', "Title"))} <span class="required-input">*</span></label>
<div id="issue-title-validation-error" class="validation-error hidden" role="alert">${escape(localize('issueTitleRequired', "Please enter a title."))}</div>
<input id="issue-title" type="text" required>
<label id="issue-title-label" for="issue-title">${escape(localize('issueTitleLabel', "Title"))} <span class="required-input">*</span></label>
<input id="issue-title" type="text" class="inline-form-control" placeholder="${escape(localize('issueTitleRequired', "Please enter a title."))}" required>
<small id="similar-issues">
<!-- To be dynamically filled -->
</small>
</div>
</div>
<div class="system-info">
<div class="input-group">
<div class="two-col">
<label for="vscode-version">${escape(localize('vscodeVersion', "VS Code Version"))}</label>
<input id="vscode-version" type="text" value="${pkg.name} ${pkg.version}" disabled/>
</div>
<div class="two-col">
<label for="os">${escape(localize('osVersion', "OS Version"))}</label>
<input id="os" type="text" value="${os.type()} ${os.arch()} ${os.release()}" disabled/>
</div>
</div>
<div id="block-container">
<div class="block block-system">
<details>
Expand Down Expand Up @@ -123,9 +109,7 @@ export default (): string => `
<!-- To be dynamically filled -->
</div>
<div class="block-info-text">
<div class="instructions">${escape(localize('githubMarkdown', "We support GitHub-flavored Markdown. You will be able to edit your issue and add screenshots when we preview it on GitHub."))}</div>
<div id="description-validation-error" class="validation-error hidden" role="alert">${escape(localize('issueDescriptionRequired', "Please enter a description."))}</div>
<textarea name="description" id="description" cols="100" rows="15" required></textarea>
<textarea name="description" id="description" cols="100" rows="12" placeholder="${escape(localize('details', "Please enter details."))}" required></textarea>
</div>
</div>
Expand Down
45 changes: 38 additions & 7 deletions src/vs/code/electron-browser/issue/media/issueReporter.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,29 @@ td {
}

.section {
margin-bottom: 2em;
margin-bottom: 1.5em;
}

#issue-type-label {
width: 12%;
}

#issue-type {
width: calc(88% - 5px);
}

#issue-title-label {
width: 10%;
display: inline-block
}

#issue-title {
width: calc(90% - 5px);
}

#similar-issues {
margin-left: 67px;
display: block;
}

/**
Expand All @@ -48,6 +70,11 @@ input[type="text"], textarea {
border-radius: .25rem;
border: 1px solid #ced4da;
}

.inline-form-control {
display: inline-block !important;
}

textarea {
overflow: auto;
resize: vertical;
Expand All @@ -73,10 +100,9 @@ button {

select {
height: calc(2.25rem + 2px);
display: block;
width: 100%;
padding: 0.375rem 0.75rem;
font-size: 1rem;
display: inline-block;
padding: 3px 3px;
font-size: 14px;
line-height: 1.5;
color: #495057;
background-color: #fff;
Expand All @@ -87,6 +113,11 @@ select {
* {
box-sizing: border-box;
}

textarea {
font-family: -apple-system, BlinkMacSystemFont, "Segoe WPC", "Segoe UI", "HelveticaNeue-Light", "Ubuntu", "Droid Sans", sans-serif;
}

html {
font-family: -apple-system, BlinkMacSystemFont, "Segoe WPC", "Segoe UI", "HelveticaNeue-Light", "Ubuntu", "Droid Sans", sans-serif;
color: #CCCCCC;
Expand All @@ -107,7 +138,7 @@ body {
.block .block-info {
width: 100%;
font-family: 'Menlo', 'Courier New', 'Courier', monospace;
font-size: 14px;
font-size: 12px;
overflow: auto;
overflow-wrap: break-word;
}
Expand Down Expand Up @@ -172,7 +203,7 @@ select, input, textarea {

summary {
border: 1px solid transparent;
padding: 10px;
padding: 0 10px;
margin-bottom: 5px;
}

Expand Down
57 changes: 52 additions & 5 deletions src/vs/platform/issue/electron-main/issueService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@ import { localize } from 'vs/nls';
import * as objects from 'vs/base/common/objects';
import { parseArgs } from 'vs/platform/environment/node/argv';
import { IIssueService, IssueReporterData } from 'vs/platform/issue/common/issue';
import { BrowserWindow, ipcMain } from 'electron';
import { BrowserWindow, ipcMain, screen } from 'electron';
import { ILaunchService } from 'vs/code/electron-main/launch';
import { buildDiagnostics, DiagnosticInfo } from 'vs/code/electron-main/diagnostics';
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { isMacintosh } from 'vs/base/common/platform';

const DEFAULT_BACKGROUND_COLOR = '#1E1E1E';

export class IssueService implements IIssueService {
_serviceBrand: any;
_issueWindow: BrowserWindow;
_parentWindow: BrowserWindow;

constructor(
private machineId: string,
Expand All @@ -35,14 +37,17 @@ export class IssueService implements IIssueService {
});

ipcMain.on('workbenchCommand', (event, arg) => {
this._issueWindow.getParentWindow().webContents.send('vscode:runAction', { id: arg });
this._parentWindow.webContents.send('vscode:runAction', { id: arg });
});

this._parentWindow = BrowserWindow.getFocusedWindow();
const position = this.getWindowPosition();
this._issueWindow = new BrowserWindow({
width: 750,
height: 1100,
width: position.width,
height: position.height,
x: position.x,
y: position.y,
title: localize('issueReporter', "Issue Reporter"),
parent: BrowserWindow.getFocusedWindow(),
backgroundColor: data.styles.backgroundColor || DEFAULT_BACKGROUND_COLOR
});

Expand All @@ -53,6 +58,48 @@ export class IssueService implements IIssueService {
return TPromise.as(null);
}

private getWindowPosition() {
// We want the new window to open on the same display that the parent is in
let displayToUse: Electron.Display;
const displays = screen.getAllDisplays();

// Single Display
if (displays.length === 1) {
displayToUse = displays[0];
}

// Multi Display
else {

// on mac there is 1 menu per window so we need to use the monitor where the cursor currently is
if (isMacintosh) {
const cursorPoint = screen.getCursorScreenPoint();
displayToUse = screen.getDisplayNearestPoint(cursorPoint);
}

// if we have a last active window, use that display for the new window
if (!displayToUse && this._parentWindow) {
displayToUse = screen.getDisplayMatching(this._parentWindow.getBounds());
}

// fallback to primary display or first display
if (!displayToUse) {
displayToUse = screen.getPrimaryDisplay() || displays[0];
}
}

let state = {
width: 750,
height: 900,
x: undefined,
y: undefined
};
state.x = displayToUse.bounds.x + (displayToUse.bounds.width / 2) - (state.width / 2);
state.y = displayToUse.bounds.y + (displayToUse.bounds.height / 2) - (state.height / 2);

return state;
}

private getStatusInfo(): TPromise<DiagnosticInfo> {
return new Promise((resolve, reject) => {
this.launchService.getMainProcessInfo().then(info => {
Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/electron-browser/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { IIssueService, IssueReporterData, IssueType, IssueReporterStyles } from
import { IThemeService, ITheme } from 'vs/platform/theme/common/themeService';
import { textLinkForeground, inputBackground, inputBorder, inputForeground, buttonBackground, buttonHoverBackground, buttonForeground, inputValidationErrorBorder, foreground, inputActiveOptionBorder } from 'vs/platform/theme/common/colorRegistry';
import { SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme';
import { getGalleryExtensionIdFromLocal } from 'vs/platform/extensionManagement/common/extensionManagementUtil';

// --- actions

Expand Down Expand Up @@ -896,7 +897,7 @@ export class OpenIssueReporterAction extends Action {

public run(): TPromise<boolean> {
return this.extensionManagementService.getInstalled(LocalExtensionType.User).then(extensions => {
const enabledExtensions = extensions.filter(extension => this.extensionEnablementService.isEnabled(extension.identifier));
const enabledExtensions = extensions.filter(extension => this.extensionEnablementService.isEnabled({ id: getGalleryExtensionIdFromLocal(extension) }));
const theme = this.themeService.getTheme();
const issueReporterData: IssueReporterData = {
styles: getIssueReporterStyles(theme),
Expand Down

0 comments on commit 5368104

Please sign in to comment.