From cc778ad69f862c4ebcec30a9b5506b9726d1a4de Mon Sep 17 00:00:00 2001 From: Cory Rivera Date: Wed, 26 Jul 2023 10:04:29 -0700 Subject: [PATCH] Use faster, editable dropdown for Collations in database dialogs (#23974) * Also fixed an issue where a manually edited text field doesn't get updated when selecting the same dropdown value from before the manual edit. --------- Co-authored-by: Charles Gagnon --- .../objectManagement/localizedConstants.ts | 2 +- .../src/objectManagement/ui/databaseDialog.ts | 12 ++++- extensions/mssql/src/ui/dialogBase.ts | 7 +-- src/sql/azdata.proposed.d.ts | 11 ++++- .../ui/editableDropdown/browser/dropdown.ts | 49 +++++++++++-------- .../ui/editableDropdown/dropdown.test.ts | 8 +++ .../modelComponents/dropdown.component.ts | 10 ++-- 7 files changed, 67 insertions(+), 32 deletions(-) diff --git a/extensions/mssql/src/objectManagement/localizedConstants.ts b/extensions/mssql/src/objectManagement/localizedConstants.ts index 62d38ad607..2e69a22a05 100644 --- a/extensions/mssql/src/objectManagement/localizedConstants.ts +++ b/extensions/mssql/src/objectManagement/localizedConstants.ts @@ -161,7 +161,7 @@ export const MemberSectionHeader = localize('objectManagement.membersLabel', "Me export const SchemaText = localize('objectManagement.schemaLabel', "Schema"); // Database -export const DatabaseExistsError = (dbName: string) => localize('objectManagement.databaseExistsError', "Database '{0}' already exists. Choose a different database name.", dbName); +export const CollationNotValidError = (collationName: string) => localize('objectManagement.collationNotValidError', "The selected collation '{0}' is not valid. Please choose a different collation.", collationName); export const CollationText = localize('objectManagement.collationLabel', "Collation"); export const RecoveryModelText = localize('objectManagement.recoveryModelLabel', "Recovery Model"); export const CompatibilityLevelText = localize('objectManagement.compatibilityLevelLabel', "Compatibility Level"); diff --git a/extensions/mssql/src/objectManagement/ui/databaseDialog.ts b/extensions/mssql/src/objectManagement/ui/databaseDialog.ts index 412fc4e0d1..f1ea34680f 100644 --- a/extensions/mssql/src/objectManagement/ui/databaseDialog.ts +++ b/extensions/mssql/src/objectManagement/ui/databaseDialog.ts @@ -134,6 +134,14 @@ export class DatabaseDialog extends ObjectManagementDialogBase { + let errors = await super.validateInput(); + if (this.viewInfo.collationNames?.length > 0 && !this.viewInfo.collationNames.some(name => name.toLowerCase() === this.objectInfo.collationName?.toLowerCase())) { + errors.push(localizedConstants.CollationNotValidError(this.objectInfo.collationName ?? '')); + } + return errors; + } + //#region Create Database private initializeGeneralSection(): azdata.GroupContainer { let containers: azdata.Component[] = []; @@ -167,7 +175,7 @@ export class DatabaseDialog extends ObjectManagementDialogBase { this.objectInfo.collationName = collationDropbox.value as string; - }, this.viewInfo.collationNames, this.viewInfo.collationNames[0]); + }, this.viewInfo.collationNames, this.viewInfo.collationNames[0], true, DefaultInputWidth, true, true); containers.push(this.createLabelInputContainer(localizedConstants.CollationText, collationDropbox)); } @@ -277,7 +285,7 @@ export class DatabaseDialog extends ObjectManagementDialogBase { this.objectInfo.collationName = newValue as string; - }, this.viewInfo.collationNames, this.objectInfo.collationName); + }, this.viewInfo.collationNames, this.objectInfo.collationName, true, DefaultInputWidth, true, true); containers.push(this.createLabelInputContainer(localizedConstants.CollationText, collationDropbox)); // Recovery Model diff --git a/extensions/mssql/src/ui/dialogBase.ts b/extensions/mssql/src/ui/dialogBase.ts index 60d075d672..28f0a9c6b0 100644 --- a/extensions/mssql/src/ui/dialogBase.ts +++ b/extensions/mssql/src/ui/dialogBase.ts @@ -27,7 +27,6 @@ export const DefaultTableListItemEnabledStateGetter: TableListItemEnabledStateGe export const DefaultTableListItemValueGetter: TableListItemValueGetter = (item: any) => [item?.toString() ?? '']; export const DefaultTableListItemComparer: TableListItemComparer = (item1: any, item2: any) => item1 === item2; - export abstract class DialogBase { protected readonly disposables: vscode.Disposable[] = []; protected readonly dialogObject: azdata.window.Dialog; @@ -287,7 +286,7 @@ export abstract class DialogBase { return this.createButtonContainer([addButton, removeButton]); } - protected createDropdown(ariaLabel: string, handler: (newValue: string) => Promise, values: string[], value: string | undefined, enabled: boolean = true, width: number = DefaultInputWidth): azdata.DropDownComponent { + protected createDropdown(ariaLabel: string, handler: (newValue: string) => Promise, values: string[], value: string | undefined, enabled: boolean = true, width: number = DefaultInputWidth, editable?: boolean, strictSelection?: boolean): azdata.DropDownComponent { // Automatically add an empty item to the beginning of the list if the current value is not specified. // This is needed when no meaningful default value can be provided. // Create a new array so that the original array isn't modified. @@ -301,7 +300,9 @@ export abstract class DialogBase { values: dropdownValues, value: value, width: width, - enabled: enabled + enabled: enabled, + editable: editable, + strictSelection: strictSelection }).component(); this.disposables.push(dropdown.onValueChanged(async () => { await handler(dropdown.value!); diff --git a/src/sql/azdata.proposed.d.ts b/src/sql/azdata.proposed.d.ts index 6a122bd7f2..a5413097cd 100644 --- a/src/sql/azdata.proposed.d.ts +++ b/src/sql/azdata.proposed.d.ts @@ -1826,14 +1826,21 @@ declare module 'azdata' { /** * Corresponds to the aria-live accessibility attribute for this component */ - ariaLive?: AriaLiveValue + ariaLive?: AriaLiveValue; } export interface ContainerProperties extends ComponentProperties { /** * Corresponds to the aria-live accessibility attribute for this component */ - ariaLive?: AriaLiveValue + ariaLive?: AriaLiveValue; + } + + export interface DropDownProperties { + /** + * Whether or not an option in the list must be selected or a "new" option can be set. Only applicable when 'editable' is true. Default false. + */ + strictSelection?: boolean; } export interface NodeInfo { diff --git a/src/sql/base/browser/ui/editableDropdown/browser/dropdown.ts b/src/sql/base/browser/ui/editableDropdown/browser/dropdown.ts index 979505d89c..3bf66a2310 100644 --- a/src/sql/base/browser/ui/editableDropdown/browser/dropdown.ts +++ b/src/sql/base/browser/ui/editableDropdown/browser/dropdown.ts @@ -112,9 +112,11 @@ export class Dropdown extends Disposable implements IListVirtualDelegate this._inputContainer = DOM.append(this._el, DOM.$('.dropdown-input.select-container')); this._inputContainer.style.width = '100%'; this._inputContainer.style.height = '100%'; + this._selectListContainer = DOM.$('div'); this._selectListContainer.style.backgroundColor = opt.contextBackground; this._selectListContainer.style.outline = `1px solid ${opt.contextBorder}`; + this._input = new InputBox(this._inputContainer, contextViewService, { validationOptions: { // @SQLTODO @@ -141,11 +143,11 @@ export class Dropdown extends Disposable implements IListVirtualDelegate })); const inputTracker = this._register(DOM.trackFocus(this._input.inputElement)); - inputTracker.onDidBlur(() => { + this._register(inputTracker.onDidBlur(() => { if (!this._selectList.isDOMFocused()) { this._onBlur.fire(); } - }); + })); /* This event listener is intended to close the expanded drop down when the ADS shell window is resized @@ -167,14 +169,12 @@ export class Dropdown extends Disposable implements IListVirtualDelegate break; case KeyCode.Escape: if (this._isDropDownVisible) { - this._input.validate(); this._onBlur.fire(); this._hideList(); e.stopPropagation(); } break; case KeyCode.Tab: - this._input.validate(); this._onBlur.fire(); this._hideList(); break; @@ -244,7 +244,7 @@ export class Dropdown extends Disposable implements IListVirtualDelegate } })); - this._input.onDidChange(e => { + this._register(this._input.onDidChange(e => { if (this._dataSource.values.length > 0) { this._dataSource.filter = e; if (this._isDropDownVisible) { @@ -254,12 +254,11 @@ export class Dropdown extends Disposable implements IListVirtualDelegate if (this.fireOnTextChange) { this.value = e; } - }); + })); - this.onBlur(() => { + this._register(this.onBlur(() => { this._hideList(); - this._input.validate(); - }); + })); this._register(this._selectList); this._register(this._input); @@ -316,6 +315,8 @@ export class Dropdown extends Disposable implements IListVirtualDelegate private _hideList(): void { this.contextViewService.hideContextView(); this._inputContainer.setAttribute('aria-expanded', 'false'); + // Show error for input box in case the user closed the dropdown without selecting anything, like by hitting Escape + this.input.validate(); } private _updateDropDownList(): void { @@ -323,17 +324,9 @@ export class Dropdown extends Disposable implements IListVirtualDelegate const selectedIndex = this._dataSource.filteredValues.indexOf(this.value); this._selectList.setSelection(selectedIndex !== -1 ? [selectedIndex] : []); - let width = this._inputContainer.clientWidth; - - // Find the longest option in the list and set our width to that (max 500px) - const longestOption = this._dataSource.filteredValues.reduce((previous, current) => { - return previous.length > current.length ? previous : current; - }, ''); - this._widthControlElement.innerText = longestOption; - const inputContainerWidth = DOM.getContentWidth(this._inputContainer); const longestOptionWidth = DOM.getTotalWidth(this._widthControlElement); - width = clamp(longestOptionWidth, inputContainerWidth, 500); + let width = clamp(longestOptionWidth, inputContainerWidth, 500); const height = Math.min(this._dataSource.filteredValues.length * this.getHeight(), this._options.maxHeight ?? 500); this._selectListContainer.style.width = `${width}px`; @@ -345,6 +338,13 @@ export class Dropdown extends Disposable implements IListVirtualDelegate if (vals) { this._dataSource.filter = undefined; this._dataSource.values = vals; + + // Find the longest option in the list to set the width of the dropdown + let longestOption = this._dataSource.values.reduce((previous, current) => { + return previous.length > current.length ? previous : current; + }, ''); + this._widthControlElement.innerText = longestOption; + if (this._isDropDownVisible) { this._updateDropDownList(); } @@ -357,9 +357,12 @@ export class Dropdown extends Disposable implements IListVirtualDelegate } public set value(val: string) { - this._input.value = val; - if (this._previousValue !== val) { + // A value can be changed either by selecting an option from the dropdown list or editing the text field directly. + // If you try to select the same dropdown value again after changing the text field directly, that change should + // still be applied, which is why we check both _previousValue and _input.value. + if (this._previousValue !== val || this._input.value !== val) { this._previousValue = val; + this._input.value = val; this._onValueChange.fire(val); } } @@ -378,7 +381,7 @@ export class Dropdown extends Disposable implements IListVirtualDelegate } private _inputValidator(value: string): IMessage | null { - if (!this._input.hasFocus() && this._input.isEnabled() && !this._selectList.isDOMFocused() && !this._dataSource.values.some(i => i === value)) { + if (this._input.isEnabled() && !this._selectList.isDOMFocused() && !this._isDropDownVisible && !this._dataSource.values.some(i => i === value)) { if (this._options.strictSelection && this._options.errorMessage) { return { content: this._options.errorMessage, @@ -418,4 +421,8 @@ export class Dropdown extends Disposable implements IListVirtualDelegate public get options(): IDropdownOptions { return this._options; } + + public set strictSelection(val: boolean | undefined) { + this._options.strictSelection = val; + } } diff --git a/src/sql/base/test/browser/ui/editableDropdown/dropdown.test.ts b/src/sql/base/test/browser/ui/editableDropdown/dropdown.test.ts index a8530ecf18..1378bbc11a 100644 --- a/src/sql/base/test/browser/ui/editableDropdown/dropdown.test.ts +++ b/src/sql/base/test/browser/ui/editableDropdown/dropdown.test.ts @@ -68,4 +68,12 @@ suite('Editable dropdown tests', () => { dropdown.input.value = options.values[0]; assert.strictEqual(count, 3, 'onValueChange event was fired with input box value change even after setting the fireOnTextChange to false'); }); + + test('selecting same dropdown value again after changing text field should update text field', () => { + const dropdown = new Dropdown(container, undefined, options); + dropdown.value = options.values[0]; + dropdown.input.value = 'NotARealValue'; + dropdown.value = options.values[0]; + assert.strictEqual(dropdown.input.value, options.values[0]); + }); }); diff --git a/src/sql/workbench/browser/modelComponents/dropdown.component.ts b/src/sql/workbench/browser/modelComponents/dropdown.component.ts index abc64f86ff..bf29dfd7db 100644 --- a/src/sql/workbench/browser/modelComponents/dropdown.component.ts +++ b/src/sql/workbench/browser/modelComponents/dropdown.component.ts @@ -81,7 +81,7 @@ export default class DropDownComponent extends ComponentBase((props) => props.placeholder, undefined); + return this.getPropertyOrDefault((props) => props.placeholder, undefined); + } + + public get strictSelection(): boolean | undefined { + return this.getPropertyOrDefault((props) => props.strictSelection, undefined); } public get validationErrorMessages(): string[] | undefined {