Add separate dialog to notebook charts for specifying chart options. (#9454)

src/sql/workbench/browser/modal/modal.ts

@@ -339,7 +339,7 @@ export abstract class Modal extends Disposable implements IThemable {
 		// Try to find focusable element in dialog pane rather than overall container. _modalBodySection contains items in the pane for a wizard.
 		// This ensures that we are setting the focus on a useful element in the form when possible.
 		const focusableElements = this._modalBodySection ?
-			this._modalBodySection.querySelectorAll('input') :
+			this._modalBodySection.querySelectorAll(tabbableElementsQuerySelector) :
 			this._bodyContainer.querySelectorAll(tabbableElementsQuerySelector);
 
 		this._focusedElementBeforeOpen = <HTMLElement>document.activeElement;

src/sql/workbench/contrib/charts/browser/actions.ts

@@ -23,6 +23,8 @@ import { assign } from 'vs/base/common/objects';
 import { IUntitledTextEditorService } from 'vs/workbench/services/untitled/common/untitledTextEditorService';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { UntitledTextEditorInput } from 'vs/workbench/services/untitled/common/untitledTextEditorInput';
+import { ChartView } from 'sql/workbench/contrib/charts/browser/chartView';
+import { ConfigureChartDialog } from 'sql/workbench/contrib/charts/browser/configureChartDialog';
 
 export interface IChartActionContext {
 	options: IInsightOptions;
@@ -106,6 +108,28 @@ export class CreateInsightAction extends Action {
 	}
 }
 
+export class ConfigureChartAction extends Action {
+	public static ID = 'chartview.configureChart';
+	public static LABEL = localize('configureChartLabel', "Configure Chart");
+	public static ICON = 'settings';
+
+	private dialog: ConfigureChartDialog;
+
+	constructor(private _chart: ChartView,
+		@IInstantiationService private readonly instantiationService: IInstantiationService) {
+		super(ConfigureChartAction.ID, ConfigureChartAction.LABEL, ConfigureChartAction.ICON);
+	}
+
+	public run(context: IChartActionContext): Promise<boolean> {
+		if (!this.dialog) {
+			this.dialog = this.instantiationService.createInstance(ConfigureChartDialog, ConfigureChartAction.LABEL, ConfigureChartAction.ID, this._chart);
+			this.dialog.render();
+		}
+		this.dialog.open();
+		return Promise.resolve(true);
+	}
+}
+
 export class CopyAction extends Action {
 	public static ID = 'chartview.copy';
 	public static LABEL = localize('copyChartLabel', "Copy as image");

src/sql/workbench/contrib/charts/browser/chartTab.ts

@@ -16,7 +16,7 @@ export class ChartTab implements IPanelTab {
 	public readonly view: ChartView;
 
 	constructor(@IInstantiationService instantiationService: IInstantiationService) {
-		this.view = instantiationService.createInstance(ChartView);
+		this.view = instantiationService.createInstance(ChartView, true);
 	}
 
 	public set queryRunner(runner: QueryRunner) {

src/sql/workbench/contrib/charts/browser/chartView.ts

@@ -20,8 +20,8 @@ import { attachSelectBoxStyler, attachInputBoxStyler } from 'vs/platform/theme/c
 import { IThemeService } from 'vs/platform/theme/common/themeService';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { isUndefinedOrNull } from 'vs/base/common/types';
-import { CreateInsightAction, CopyAction, SaveImageAction, IChartActionContext } from 'sql/workbench/contrib/charts/browser/actions';
-import { Taskbar } from 'sql/base/browser/ui/taskbar/taskbar';
+import { CreateInsightAction, CopyAction, SaveImageAction, IChartActionContext, ConfigureChartAction } from 'sql/workbench/contrib/charts/browser/actions';
+import { Taskbar, ITaskbarContent } from 'sql/base/browser/ui/taskbar/taskbar';
 import { Checkbox } from 'sql/base/browser/ui/checkbox/checkbox';
 import { IInsightOptions, ChartType } from 'sql/workbench/contrib/charts/common/interfaces';
 import { ChartState } from 'sql/workbench/common/editor/query/chartState';
@@ -57,6 +57,7 @@ export class ChartView extends Disposable implements IPanelView {
 	private _createInsightAction: CreateInsightAction;
 	private _copyAction: CopyAction;
 	private _saveAction: SaveImageAction;
+	private _configureChartAction: ConfigureChartAction;
 
 	private _state: ChartState;
 
@@ -68,7 +69,7 @@ export class ChartView extends Disposable implements IPanelView {
 	/** parent container */
 	private container: HTMLElement;
 	/** container for the options controls */
-	private optionsControl: HTMLElement;
+	public readonly optionsControl: HTMLElement;
 	/** container for type specific controls */
 	private typeControls: HTMLElement;
 	/** container for the insight */
@@ -82,6 +83,7 @@ export class ChartView extends Disposable implements IPanelView {
 	private optionMap: { [x: string]: { element: HTMLElement; set: (val) => void } } = {};
 
 	constructor(
+		private readonly _renderOptionsInline: boolean,
 		@IContextViewService private _contextViewService: IContextViewService,
 		@IThemeService private _themeService: IThemeService,
 		@IInstantiationService private _instantiationService: IInstantiationService,
@@ -90,6 +92,7 @@ export class ChartView extends Disposable implements IPanelView {
 		super();
 		this.taskbarContainer = DOM.$('div.taskbar-container');
 		this.taskbar = new Taskbar(this.taskbarContainer);
+
 		this.optionsControl = DOM.$('div.options-container');
 		const generalControls = DOM.$('div.general-controls');
 		this.optionsControl.appendChild(generalControls);
@@ -100,7 +103,12 @@ export class ChartView extends Disposable implements IPanelView {
 		this._copyAction = this._instantiationService.createInstance(CopyAction);
 		this._saveAction = this._instantiationService.createInstance(SaveImageAction);
 
-		this.taskbar.setContent([{ action: this._createInsightAction }]);
+		if (this._renderOptionsInline) {
+			this.taskbar.setContent([{ action: this._createInsightAction }]);
+		} else {
+			this._configureChartAction = this._instantiationService.createInstance(ConfigureChartAction, this);
+			this.taskbar.setContent([{ action: this._createInsightAction }, { action: this._configureChartAction }]);
+		}
 
 		const self = this;
 		this.options = new Proxy(this.options, {
@@ -165,7 +173,9 @@ export class ChartView extends Disposable implements IPanelView {
 			this.container.appendChild(this.taskbarContainer);
 			this.container.appendChild(this.chartingContainer);
 			this.chartingContainer.appendChild(this.insightContainer);
-			this.chartingContainer.appendChild(this.optionsControl);
+			if (this._renderOptionsInline) {
+				this.chartingContainer.appendChild(this.optionsControl);
+			}
 			this.insight = new Insight(this.insightContainer, this.options, this._instantiationService);
 		}
 
@@ -275,16 +285,21 @@ export class ChartView extends Disposable implements IPanelView {
 	}
 
 	private updateActionbar() {
+		let actions: ITaskbarContent[];
 		if (this.insight && this.insight.isCopyable) {
 			this.taskbar.context = { insight: this.insight.insight, options: this.options };
-			this.taskbar.setContent([
+			actions = [
 				{ action: this._createInsightAction },
 				{ action: this._copyAction },
 				{ action: this._saveAction }
-			]);
+			];
 		} else {
-			this.taskbar.setContent([{ action: this._createInsightAction }]);
+			actions = [{ action: this._createInsightAction }];
 		}
+		if (!this._renderOptionsInline) {
+			actions.push({ action: this._configureChartAction });
+		}
+		this.taskbar.setContent(actions);
 	}
 
 	private createOption(option: IChartOption, container: HTMLElement) {
@@ -318,6 +333,7 @@ export class ChartView extends Disposable implements IPanelView {
 			case ControlType.combo:
 				//pass options into changeAltNames in order for SelectBox to show user-friendly names.
 				let dropdown = new SelectBox(option.displayableOptions || this.changeToAltNames(option.options), undefined, this._contextViewService);
+				dropdown.setAriaLabel(option.label);
 				dropdown.select(option.options.indexOf(value));
 				dropdown.render(optionInput);
 				dropdown.onDidSelect(e => {
@@ -337,6 +353,7 @@ export class ChartView extends Disposable implements IPanelView {
 				break;
 			case ControlType.input:
 				let input = new InputBox(optionInput, this._contextViewService);
+				input.setAriaLabel(option.label);
 				input.value = value || '';
 				input.onDidChange(e => {
 					if (this.options[option.configEntry] !== e) {
@@ -355,6 +372,7 @@ export class ChartView extends Disposable implements IPanelView {
 				break;
 			case ControlType.numberInput:
 				let numberInput = new InputBox(optionInput, this._contextViewService, { type: 'number' });
+				numberInput.setAriaLabel(option.label);
 				numberInput.value = value || '';
 				numberInput.onDidChange(e => {
 					if (this.options[option.configEntry] !== Number(e)) {
@@ -373,6 +391,7 @@ export class ChartView extends Disposable implements IPanelView {
 				break;
 			case ControlType.dateInput:
 				let dateInput = new InputBox(optionInput, this._contextViewService, { type: 'datetime-local' });
+				dateInput.setAriaLabel(option.label);
 				dateInput.value = value || '';
 				dateInput.onDidChange(e => {
 					if (this.options[option.configEntry] !== e) {

src/sql/workbench/contrib/charts/browser/configureChartDialog.ts

@@ -0,0 +1,57 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { IThemeService } from 'vs/platform/theme/common/themeService';
+import { localize } from 'vs/nls';
+import { ChartView } from 'sql/workbench/contrib/charts/browser/chartView';
+import { Modal } from 'sql/workbench/browser/modal/modal';
+import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService';
+import { IAdsTelemetryService } from 'sql/platform/telemetry/common/telemetry';
+import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
+import { IClipboardService } from 'vs/platform/clipboard/common/clipboardService';
+import { ILogService } from 'vs/platform/log/common/log';
+import { ITextResourcePropertiesService } from 'vs/editor/common/services/textResourceConfigurationService';
+import { attachModalDialogStyler } from 'sql/workbench/common/styler';
+import { attachButtonStyler } from 'vs/platform/theme/common/styler';
+
+export class ConfigureChartDialog extends Modal {
+	constructor(
+		title: string,
+		name: string,
+		private _chart: ChartView,
+		@IWorkbenchLayoutService layoutService: IWorkbenchLayoutService,
+		@IThemeService themeService: IThemeService,
+		@IAdsTelemetryService telemetryService: IAdsTelemetryService,
+		@IContextKeyService contextKeyService: IContextKeyService,
+		@IClipboardService clipboardService: IClipboardService,
+		@ILogService logService: ILogService,
+		@ITextResourcePropertiesService textResourcePropertiesService: ITextResourcePropertiesService
+	) {
+		super(title, name, telemetryService, layoutService, clipboardService, themeService, logService, textResourcePropertiesService, contextKeyService, undefined);
+	}
+
+	public open() {
+		this.show();
+	}
+
+	public render() {
+		super.render();
+		attachModalDialogStyler(this, this._themeService);
+
+		let closeButton = this.addFooterButton(localize('configureChartDialog.close', "Close"), () => this.close());
+		attachButtonStyler(closeButton, this._themeService);
+	}
+
+	protected renderBody(container: HTMLElement) {
+		container.appendChild(this._chart.optionsControl);
+	}
+
+	protected layout(height?: number): void {
+	}
+
+	public close() {
+		this.hide();
+	}
+}

src/sql/workbench/contrib/charts/browser/media/chartView.css

@@ -9,6 +9,7 @@
 	display: flex;
 	flex-direction: column;
 	overflow: scroll;
+	min-height: 400px;
 }
 
 .actionbar-container {
@@ -26,6 +27,7 @@
 }
 
 .options-container {
+	padding: 20px;
 	min-width: 250px;
 	padding-right: 10px;
 }

src/sql/workbench/contrib/charts/test/browser/chartView.test.ts

@@ -14,22 +14,32 @@ import { TestNotificationService } from 'vs/platform/notification/test/common/te
 
 suite('Chart View', () => {
 	test('initializes without error', () => {
-		const chartview = createChartView();
+		const chartview = createChartView(true);
 		assert(chartview);
 	});
 
 	test('renders without error', () => {
-		const chartview = createChartView();
+		const chartview = createChartView(true);
+		chartview.render(document.createElement('div'));
+	});
+
+	test('initializes without error - without options', () => {
+		const chartview = createChartView(false);
+		assert(chartview);
+	});
+
+	test('renders without error - without options', () => {
+		const chartview = createChartView(false);
 		chartview.render(document.createElement('div'));
 	});
 });
 
-function createChartView(): ChartView {
+function createChartView(renderOptions: boolean): ChartView {
 	const layoutService = new TestLayoutService();
 	const contextViewService = new ContextViewService(layoutService);
 	const themeService = new TestThemeService();
 	const instantiationService = new TestInstantiationService();
 	const notificationService = new TestNotificationService();
 	instantiationService.stub(IThemeService, themeService);
-	return new ChartView(contextViewService, themeService, instantiationService, notificationService);
+	return new ChartView(renderOptions, contextViewService, themeService, instantiationService, notificationService);
 }

src/sql/workbench/contrib/notebook/browser/outputs/gridOutput.component.ts

@@ -126,7 +126,7 @@ class DataResourceTable extends GridTableBase<any> {
 	) {
 		super(state, createResultSet(source), contextMenuService, instantiationService, editorService, untitledEditorService, configurationService);
 		this._gridDataProvider = this.instantiationService.createInstance(DataResourceDataProvider, source, this.resultSet, this.documentUri);
-		this._chart = this.instantiationService.createInstance(ChartView);
+		this._chart = this.instantiationService.createInstance(ChartView, false);
 	}
 
 	get gridDataProvider(): IGridDataProvider {
@@ -389,13 +389,13 @@ export class NotebookChartAction extends ToggleableAction {
 	public static SHOWCHART_LABEL = localize('notebook.showChart', "Show chart");
 	public static SHOWCHART_ICON = 'viewChart';
 
-	public static HIDECHART_LABEL = localize('notebook.hideChart', "Hide chart");
-	public static HIDECHART_ICON = 'close';
+	public static SHOWTABLE_LABEL = localize('notebook.showTable', "Show table");
+	public static SHOWTABLE_ICON = 'table';
 
 	constructor(private resourceTable: DataResourceTable) {
 		super(NotebookChartAction.ID, {
-			toggleOnLabel: NotebookChartAction.HIDECHART_LABEL,
-			toggleOnClass: NotebookChartAction.HIDECHART_ICON,
+			toggleOnLabel: NotebookChartAction.SHOWTABLE_LABEL,
+			toggleOnClass: NotebookChartAction.SHOWTABLE_ICON,
 			toggleOffLabel: NotebookChartAction.SHOWCHART_LABEL,
 			toggleOffClass: NotebookChartAction.SHOWCHART_ICON,
 			isOn: false

src/sql/workbench/services/objectExplorer/browser/media/objectTypes/objecttypes.css

@@ -5,7 +5,10 @@
 
 .vs .icon.table,
 .vs-dark .icon.table,
-.hc-black .icon.table {
+.hc-black .icon.table,
+.vs .codicon.table,
+.vs-dark .codicon.table,
+.hc-black .codicon.table {
 	background: url("Table.svg") center center no-repeat;
 }