From 1d539b46bca63cf6c0603433951de54233fb262b Mon Sep 17 00:00:00 2001 From: Benjin Dubishar Date: Mon, 14 Oct 2024 17:00:36 -0700 Subject: [PATCH] Fixing smattering of Azure Browse bugs (#18197) * fixing ability to type into filter boxes * adding validation for combobox dropdowns * loc updates * clearing validation message * lifting state to handle automatic first item selection * Adding placeholders * updating loc * initial * Revert "initial" This reverts commit 0770b79573bf65b9ee2a8324f3730be72fd046e4. * adding icons for connection input modes * adding docstrings * swapping to b/w icon for Azure * updating string * loc updates * fixing string * loc updates --- localization/l10n/bundle.l10n.json | 18 ++ localization/xliff/vscode-mssql.xlf | 26 +++ src/reactviews/common/locConstants.ts | 32 +++- src/reactviews/common/utils.ts | 8 + src/reactviews/media/azure-inverse.svg | 3 + src/reactviews/media/azure.svg | 3 + .../ConnectionDialog/azureBrowsePage.tsx | 167 ++++++++++++++---- .../connectionPageContainer.tsx | 65 ++++++- .../pages/ExecutionPlan/queryPlanSetup.ts | 10 +- 9 files changed, 281 insertions(+), 51 deletions(-) create mode 100644 src/reactviews/media/azure-inverse.svg create mode 100644 src/reactviews/media/azure.svg diff --git a/localization/l10n/bundle.l10n.json b/localization/l10n/bundle.l10n.json index 7a5a957b..593f7ada 100644 --- a/localization/l10n/bundle.l10n.json +++ b/localization/l10n/bundle.l10n.json @@ -189,16 +189,34 @@ "Browse Azure": "Browse Azure", "Recent Connections": "Recent Connections", "Subscription": "Subscription", + "subscription": "subscription", "Resource Group": "Resource Group", + "resource group": "resource group", "Location": "Location", + "location": "location", "Server": "Server", + "server": "server", "Database": "Database", + "database": "database", "Filter Azure subscriptions": "Filter Azure subscriptions", "Connection Error": "Connection Error", "Encryption was enabled on this connection; review your SSL and certificate configuration for the target SQL Server, or enable 'Trust server certificate' in the connection dialog.": "Encryption was enabled on this connection; review your SSL and certificate configuration for the target SQL Server, or enable 'Trust server certificate' in the connection dialog.", "Note: A self-signed certificate offers only limited protection and is not a recommended practice for production environments. Do you want to enable 'Trust server certificate' on this connection and retry?": "Note: A self-signed certificate offers only limited protection and is not a recommended practice for production environments. Do you want to enable 'Trust server certificate' on this connection and retry?", "Read more": "Read more", "Enable 'Trust Server Certificate'": "Enable 'Trust Server Certificate'", + "Select a {0} for filtering/{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'": { + "message": "Select a {0} for filtering", + "comment": [ + "{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'" + ] + }, + "Select a valid {0} from the dropdown/{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'": { + "message": "Select a valid {0} from the dropdown", + "comment": [ + "{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'" + ] + }, + "Default": "Default", "Query {0}: Query cost (relative to the script): {1}%/{0} is the query number{1} is the query cost": { "message": "Query {0}: Query cost (relative to the script): {1}%", "comment": [ diff --git a/localization/xliff/vscode-mssql.xlf b/localization/xliff/vscode-mssql.xlf index bf724f41..3466d7fa 100644 --- a/localization/xliff/vscode-mssql.xlf +++ b/localization/xliff/vscode-mssql.xlf @@ -335,6 +335,9 @@ Database name + + Default + Default Value @@ -1036,6 +1039,14 @@ Select a tenant + + Select a valid {0} from the dropdown + {0} is the type of the dropdown's contents, e.g 'resource group' or 'server' + + + Select a {0} for filtering + {0} is the type of the dropdown's contents, e.g 'resource group' or 'server' + Select all @@ -1344,6 +1355,9 @@ authenticationType + + database + encrypt @@ -1353,9 +1367,21 @@ intelliSenseUpdated + + location + macOS Sierra or newer is required to use this feature. + + resource group + + + server + + + subscription + test diff --git a/src/reactviews/common/locConstants.ts b/src/reactviews/common/locConstants.ts index 96969748..f9cf6471 100644 --- a/src/reactviews/common/locConstants.ts +++ b/src/reactviews/common/locConstants.ts @@ -150,11 +150,16 @@ export class LocConstants { connectionString: l10n.t("Connection String"), browseAzure: l10n.t("Browse Azure"), recentConnections: l10n.t("Recent Connections"), - subscription: l10n.t("Subscription"), - resourceGroup: l10n.t("Resource Group"), - location: l10n.t("Location"), - server: l10n.t("Server"), - database: l10n.t("Database"), + subscriptionLabel: l10n.t("Subscription"), + subscription: l10n.t("subscription"), + resourceGroupLabel: l10n.t("Resource Group"), + resourceGroup: l10n.t("resource group"), + locationLabel: l10n.t("Location"), + location: l10n.t("location"), + serverLabel: l10n.t("Server"), + server: l10n.t("server"), + databaseLabel: l10n.t("Database"), + database: l10n.t("database"), filterSubscriptions: l10n.t("Filter Azure subscriptions"), connectionErrorTitle: l10n.t("Connection Error"), trustServerCertMessage: l10n.t( @@ -168,6 +173,23 @@ export class LocConstants { "Enable 'Trust Server Certificate'", ), close: l10n.t("Close"), + azureFilterPlaceholder: (dropdownContentType: string) => + l10n.t({ + message: "Select a {0} for filtering", + args: [dropdownContentType], + comment: [ + "{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'", + ], + }), + invalidAzureBrowse: (dropdownContentType: string) => + l10n.t({ + message: "Select a valid {0} from the dropdown", + args: [dropdownContentType], + comment: [ + "{0} is the type of the dropdown's contents, e.g 'resource group' or 'server'", + ], + }), + default: l10n.t("Default"), }; } diff --git a/src/reactviews/common/utils.ts b/src/reactviews/common/utils.ts index f48308d5..74d8a641 100644 --- a/src/reactviews/common/utils.ts +++ b/src/reactviews/common/utils.ts @@ -41,3 +41,11 @@ export function getVscodeThemeType(theme: Theme): string { return "light"; } } + +export function themeType(theme: Theme): string { + const themeType = getVscodeThemeType(theme); + if (themeType !== "light") { + return "dark"; + } + return themeType; +} diff --git a/src/reactviews/media/azure-inverse.svg b/src/reactviews/media/azure-inverse.svg new file mode 100644 index 00000000..9267cce7 --- /dev/null +++ b/src/reactviews/media/azure-inverse.svg @@ -0,0 +1,3 @@ + + + diff --git a/src/reactviews/media/azure.svg b/src/reactviews/media/azure.svg new file mode 100644 index 00000000..73c5f28d --- /dev/null +++ b/src/reactviews/media/azure.svg @@ -0,0 +1,3 @@ + + + diff --git a/src/reactviews/pages/ConnectionDialog/azureBrowsePage.tsx b/src/reactviews/pages/ConnectionDialog/azureBrowsePage.tsx index 5793d8b0..af3800c7 100644 --- a/src/reactviews/pages/ConnectionDialog/azureBrowsePage.tsx +++ b/src/reactviews/pages/ConnectionDialog/azureBrowsePage.tsx @@ -13,6 +13,9 @@ import { Combobox, Spinner, makeStyles, + ComboboxProps, + OptionOnSelectData, + SelectionEvents, } from "@fluentui/react-components"; import { Filter16Filled } from "@fluentui/react-icons"; import { FormField, useFormStyles } from "../../common/forms/form.component"; @@ -39,26 +42,33 @@ export const AzureBrowsePage = () => { const [selectedSubscription, setSelectedSubscription] = useState< string | undefined >(undefined); + const [subscriptionValue, setSubscriptionValue] = useState(""); const [resourceGroups, setResourceGroups] = useState([]); const [selectedResourceGroup, setSelectedResourceGroup] = useState< string | undefined >(undefined); + const [resourceGroupValue, setResourceGroupValue] = useState(""); const [locations, setLocations] = useState([]); const [selectedLocation, setSelectedLocation] = useState< string | undefined >(undefined); + const [locationValue, setLocationValue] = useState(""); const [servers, setServers] = useState([]); const [selectedServer, setSelectedServer] = useState( undefined, ); + const [serverValue, setServerValue] = useState(""); const [databases, setDatabases] = useState([]); const [selectedDatabase, setSelectedDatabase] = useState< string | undefined >(undefined); + const [databaseValue, setDatabaseValue] = useState(""); + + // #region Effects useEffect(() => { const subs = removeDuplicates( @@ -148,10 +158,19 @@ export const AzureBrowsePage = () => { ); setServers(srvs.sort()); - // if current selection is no longer in the list of options, - // set selection to undefined (if multiple options) or the only option (if only one) - if (selectedServer && !srvs.includes(selectedServer)) { - setSelectedServer(srvs.length === 1 ? srvs[0] : undefined); + // intentionally cleared + if (selectedServer === "") { + setServerValue(""); + } else { + // if there is no current selection or if the selection is no longer in the list of options (due to changed filters), + // set selection to the first option (if any) + if ( + selectedServer === undefined || + !srvs.includes(selectedServer) + ) { + setSelectedServer(srvs.length > 0 ? srvs[0] : undefined); + setServerValue(srvs.length > 0 ? srvs[0] : ""); + } } }, [locations, selectedLocation, context.state.azureServers]); @@ -174,6 +193,8 @@ export const AzureBrowsePage = () => { setSelectedDatabase(dbs.length === 1 ? dbs[0] : undefined); }, [selectedServer]); + // #endregion + function setConnectionProperty( propertyName: keyof IConnectionDialogProfile, value: string, @@ -184,7 +205,7 @@ export const AzureBrowsePage = () => { return (
@@ -204,7 +225,10 @@ export const AzureBrowsePage = () => { } content={{ valueList: subscriptions, - setValue: (sub) => { + value: subscriptionValue, + setValue: setSubscriptionValue, + selection: selectedSubscription, + setSelection: (sub) => { setSelectedSubscription(sub); if (sub === undefined) { @@ -232,29 +256,53 @@ export const AzureBrowsePage = () => { context.loadAzureServers(subId); }, - currentValue: selectedSubscription, + placeholder: Loc.connectionDialog.azureFilterPlaceholder( + Loc.connectionDialog.subscription, + ), + invalidOptionErrorMessage: + Loc.connectionDialog.invalidAzureBrowse( + Loc.connectionDialog.subscription, + ), }} /> { } content={{ valueList: servers, - setValue: (srv) => { + value: serverValue, + setValue: setServerValue, + selection: selectedServer, + setSelection: (srv) => { setSelectedServer(srv); setConnectionProperty( "server", srv ? srv + ".database.windows.net" : "", ); }, - currentValue: selectedServer, + invalidOptionErrorMessage: + Loc.connectionDialog.invalidAzureBrowse( + Loc.connectionDialog.server, + ), }} /> @@ -288,15 +342,22 @@ export const AzureBrowsePage = () => { props={{ orientation: "horizontal" }} /> { + value: databaseValue, + setValue: setDatabaseValue, + selection: selectedDatabase, + setSelection: (db) => { setSelectedDatabase(db); setConnectionProperty("database", db ?? ""); }, - currentValue: selectedDatabase, + placeholder: `<${Loc.connectionDialog.default}>`, + invalidOptionErrorMessage: + Loc.connectionDialog.invalidAzureBrowse( + Loc.connectionDialog.database, + ), }} /> {context.state.connectionComponents.mainOptions @@ -368,24 +429,66 @@ const AzureBrowseDropdown = ({ clearable, content, decoration, + props, }: { label: string; required?: boolean; clearable?: boolean; content: { + /** list of valid values for the combo box */ valueList: string[]; - setValue: (value: string | undefined) => void; - currentValue?: string; + /** currently-selected value from `valueList` */ + selection?: string; + /** callback when the user has selected a value from `valueList` */ + setSelection: (value: string | undefined) => void; + /** currently-entered text in the combox, may not be a valid selection value if the user is typing */ + value: string; + /** callback when the user types in the combobox */ + setValue: (value: string) => void; + /** placeholder text for the combobox */ + placeholder?: string; + /** message displayed if focus leaves this combobox and `value` is not a valid value from `valueList` */ + invalidOptionErrorMessage: string; }; decoration?: JSX.Element; + props?: Partial; }) => { const formStyles = useFormStyles(); const decorationStyles = useFieldDecorationStyles(); + const [validationMessage, setValidationMessage] = useState(""); - const onInput = (ev: React.ChangeEvent) => { - content.setValue(ev.target.value); + // clear validation message as soon as value is valid + useEffect(() => { + if (content.valueList.includes(content.value)) { + setValidationMessage(""); + } + }, [content.value]); + + // only display validation error if focus leaves the field and the value is not valid + const onBlur = () => { + if (content.value) { + setValidationMessage( + content.valueList.includes(content.value) + ? "" + : content.invalidOptionErrorMessage, + ); + } }; + const onOptionSelect: ( + _: SelectionEvents, + data: OptionOnSelectData, + ) => void = (_, data: OptionOnSelectData) => { + content.setSelection( + data.selectedOptions.length > 0 ? data.selectedOptions[0] : "", + ); + content.setValue(data.optionText ?? ""); + }; + + function onInput(ev: React.ChangeEvent) { + content.setValue(ev.target.value); + } + return (
{ - if (data.optionValue === content.currentValue) { - return; - } - - content.setValue(data.optionValue); - }} + onOptionSelect={onOptionSelect} + placeholder={content.placeholder} + clearable={clearable} > {content.valueList.map((val, idx) => { return ( diff --git a/src/reactviews/pages/ConnectionDialog/connectionPageContainer.tsx b/src/reactviews/pages/ConnectionDialog/connectionPageContainer.tsx index 230fd68e..d2bc32ea 100644 --- a/src/reactviews/pages/ConnectionDialog/connectionPageContainer.tsx +++ b/src/reactviews/pages/ConnectionDialog/connectionPageContainer.tsx @@ -10,7 +10,10 @@ import { MessageBar, Radio, RadioGroup, + Image, + Theme, } from "@fluentui/react-components"; +import { SlideText20Regular, Form20Regular } from "@fluentui/react-icons"; import { ConnectionDialogContextProps, IConnectionDialogProfile, @@ -26,6 +29,7 @@ import { FormItemSpec } from "../../common/forms/form"; import { locConstants } from "../../common/locConstants"; import { AzureBrowsePage } from "./azureBrowsePage"; import { TrustServerCertificateDialog } from "./components/trustServerCertificateDialog.component"; +import { themeType } from "../../common/utils"; function renderContent( connectionDialogContext: ConnectionDialogContextProps, @@ -44,6 +48,15 @@ export const ConnectionInfoFormContainer = () => { const context = useContext(ConnectionDialogContext)!; const formStyles = useFormStyles(); + function azureIcon(colorTheme: Theme) { + const theme = themeType(colorTheme); + const saveIcon = + theme === "dark" + ? require("../../media/azure-inverse.svg") + : require("../../media/azure.svg"); + return saveIcon; + } + return (
@@ -78,19 +91,63 @@ export const ConnectionInfoFormContainer = () => { > + + { + locConstants.connectionDialog + .parameters + } +
+ } /> + + { + locConstants.connectionDialog + .connectionString + } +
} /> + Azure + { + locConstants.connectionDialog + .browseAzure + } +
} /> diff --git a/src/reactviews/pages/ExecutionPlan/queryPlanSetup.ts b/src/reactviews/pages/ExecutionPlan/queryPlanSetup.ts index 2046a745..6eaf46ae 100644 --- a/src/reactviews/pages/ExecutionPlan/queryPlanSetup.ts +++ b/src/reactviews/pages/ExecutionPlan/queryPlanSetup.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { Theme } from "@fluentui/react-components"; -import { getVscodeThemeType } from "../../common/utils"; +import { themeType } from "../../common/utils"; const iterator_catch_all = require("./icons/iterator_catch_all.png"); const cursor_catch_all = require("./icons/cursor_catch_all.png"); @@ -398,14 +398,6 @@ export function getCollapseExpandPaths() { }; } -function themeType(theme: Theme): string { - const themeType = getVscodeThemeType(theme); - if (themeType !== "light") { - return "dark"; - } - return themeType; -} - export const save = (colorTheme: Theme) => { const theme = themeType(colorTheme); const saveIcon =