From f467476acbab59208be657d8de15d2c6996705ab Mon Sep 17 00:00:00 2001 From: Anemy Date: Thu, 27 Jan 2022 19:13:59 -0500 Subject: [PATCH 1/7] Add error and warning indicators to tabs --- .../advanced-connection-options.tsx | 8 +- .../advanced-options-tabs.spec.tsx | 96 +++++++++++++++++++ .../advanced-options-tabs.tsx | 62 +++++++++++- .../src/components/connect-form.tsx | 3 +- .../src/types/connect-form-tab.ts | 6 ++ .../connect-form/src/utils/validation.spec.ts | 14 +++ packages/connect-form/src/utils/validation.ts | 13 +++ 7 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx create mode 100644 packages/connect-form/src/types/connect-form-tab.ts diff --git a/packages/connect-form/src/components/advanced-connection-options.tsx b/packages/connect-form/src/components/advanced-connection-options.tsx index 11e1c8ffbb7..9745f3d2943 100644 --- a/packages/connect-form/src/components/advanced-connection-options.tsx +++ b/packages/connect-form/src/components/advanced-connection-options.tsx @@ -9,7 +9,10 @@ import { ConnectionOptions } from 'mongodb-data-service'; import AdvancedOptionsTabs from './advanced-options-tabs/advanced-options-tabs'; import { UpdateConnectionFormField } from '../hooks/use-connect-form'; -import { ConnectionFormError } from '../utils/validation'; +import { + ConnectionFormError, + ConnectionFormWarning, +} from '../utils/validation'; const disabledOverlayStyles = css({ position: 'absolute', @@ -32,11 +35,13 @@ function AdvancedConnectionOptions({ errors, updateConnectionFormField, connectionOptions, + warnings, }: { errors: ConnectionFormError[]; disabled: boolean; updateConnectionFormField: UpdateConnectionFormField; connectionOptions: ConnectionOptions; + warnings: ConnectionFormWarning[]; }): React.ReactElement { return ( @@ -51,6 +56,7 @@ function AdvancedConnectionOptions({ errors={errors} updateConnectionFormField={updateConnectionFormField} connectionOptions={connectionOptions} + warnings={warnings} /> diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx new file mode 100644 index 00000000000..9d8a68c56ff --- /dev/null +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx @@ -0,0 +1,96 @@ +import React from 'react'; +import { cleanup, render, screen } from '@testing-library/react'; +import { expect } from 'chai'; +import sinon from 'sinon'; + +import AdvancedOptionsTabs from './advanced-options-tabs'; + +const testUrl = 'mongodb+srv://0ranges:p!neapp1es@localhost/?ssl=true'; + +describe('AdvancedOptionsTabs Component', function () { + let updateConnectionFormFieldSpy: sinon.SinonSpy; + + beforeEach(function () { + updateConnectionFormFieldSpy = sinon.spy(); + }); + + afterEach(cleanup); + + it('should render all of the tabs', function () { + render( + + ); + + [ + 'General', + 'Authentication', + 'TLS/SSL', + 'Proxy/SSH Tunnel', + 'Advanced', + ].forEach((tabName) => { + expect(screen.getByText(tabName)).to.be.visible; + }); + }); + + it('should have only the tab with an error on it showing an error', function () { + render( + + ); + + ['General', 'Authentication', 'TLS/SSL', 'Proxy/SSH Tunnel'].forEach( + (tabName) => { + expect(screen.queryByTestId(`${tabName}-tab-has-error`)).to.not.exist; + } + ); + expect(screen.getAllByTestId('Advanced-tab-has-error')[0]).to.be.visible; + }); + + it('the tab with a warning should have the warning test id (and hopefully the visual indicator)', function () { + render( + + ); + + ['General', 'TLS/SSL', 'Proxy/SSH Tunnel', 'Advanced'].forEach( + (tabName) => { + expect(screen.queryByTestId(`${tabName}-tab-has-warning`)).to.not.exist; + } + ); + expect(screen.getAllByTestId('Authentication-tab-has-warning')[0]).to.be + .visible; + }); +}); diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index 07c2b66d4c3..55aa4cdf71c 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -1,5 +1,5 @@ import React, { useState, useMemo } from 'react'; -import { Tabs, Tab, spacing, css } from '@mongodb-js/compass-components'; +import { Tabs, Tab, spacing, css, cx } from '@mongodb-js/compass-components'; import ConnectionStringUrl from 'mongodb-connection-string-url'; import { ConnectionOptions } from 'mongodb-data-service'; @@ -9,14 +9,47 @@ import ProxyAndSshTunnelTab from './ssh-tunnel-tab/proxy-and-ssh-tunnel-tab'; import TLSTab from './tls-ssl-tab/tls-ssl-tab'; import AdvancedTab from './advanced-tab/advanced-tab'; import { UpdateConnectionFormField } from '../../hooks/use-connect-form'; -import { ConnectionFormError } from '../../utils/validation'; +import { + ConnectionFormError, + ConnectionFormWarning, +} from '../../utils/validation'; import { defaultConnectionString } from '../../constants/default-connection'; +import { ConnectFormTab } from '../../types/connect-form-tab'; const tabsStyles = css({ marginTop: spacing[1], }); + +const tabWithErrorStyle = css({ + position: 'relative', + '[role=tab]&::before': { + position: 'absolute', + top: 5, + right: 5, + content: '""', + width: 7, + height: 7, + backgroundColor: 'red', + borderRadius: '50%', + }, +}); + +const tabWithWarningStyle = css({ + position: 'relative', + 'button &::before': { + position: 'absolute', + top: spacing[1], + right: spacing[1], + content: '""', + width: spacing[2], + height: spacing[2], + backgroundColor: 'yellow', + borderRadius: '50%', + }, +}); + interface TabObject { - name: string; + name: ConnectFormTab; component: React.FunctionComponent<{ errors: ConnectionFormError[]; connectionStringUrl: ConnectionStringUrl; @@ -29,10 +62,12 @@ function AdvancedOptionsTabs({ errors, updateConnectionFormField, connectionOptions, + warnings, }: { errors: ConnectionFormError[]; updateConnectionFormField: UpdateConnectionFormField; connectionOptions: ConnectionOptions; + warnings: ConnectionFormWarning[]; }): React.ReactElement { const [activeTab, setActiveTab] = useState(0); @@ -63,8 +98,27 @@ function AdvancedOptionsTabs({ {tabs.map((tabObject: TabObject, idx: number) => { const TabComponent = tabObject.component; + const tabHasError = !!errors.find( + (error) => error.fieldTab === tabObject.name + ); + // If the tab has an error, don't show the warning indicator. + const tabHasWarning = + !tabHasError && + !!warnings.find((warnings) => warnings.fieldTab === tabObject.name); + return ( - + )} Date: Thu, 27 Jan 2022 19:22:05 -0500 Subject: [PATCH 2/7] clean up style definition --- .../advanced-options-tabs.tsx | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index 55aa4cdf71c..017b48eaec9 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -20,31 +20,28 @@ const tabsStyles = css({ marginTop: spacing[1], }); -const tabWithErrorStyle = css({ +const tabWithIndicatorStyles = css({ position: 'relative', '[role=tab]&::before': { position: 'absolute', - top: 5, - right: 5, + top: spacing[1], + right: spacing[1], content: '""', - width: 7, - height: 7, - backgroundColor: 'red', + width: spacing[2], + height: spacing[2], borderRadius: '50%', }, }); +const tabWithErrorStyle = css({ + '[role=tab]&::before': { + backgroundColor: 'red', + }, +}); + const tabWithWarningStyle = css({ - position: 'relative', 'button &::before': { - position: 'absolute', - top: spacing[1], - right: spacing[1], - content: '""', - width: spacing[2], - height: spacing[2], backgroundColor: 'yellow', - borderRadius: '50%', }, }); @@ -112,6 +109,7 @@ function AdvancedOptionsTabs({ tabHasError ? '-has-error' : '' }${tabHasWarning ? '-has-warning' : ''}`} className={cx({ + [tabWithIndicatorStyles]: tabHasError || tabHasWarning, [tabWithErrorStyle]: tabHasError, [tabWithWarningStyle]: tabHasWarning, })} From bf60181b19267cce2ad91de1505864ab9ff5ede5 Mon Sep 17 00:00:00 2001 From: Anemy Date: Fri, 28 Jan 2022 11:54:28 -0500 Subject: [PATCH 3/7] remove warning indicator for tabs since it isnt used yet, better test ids --- .../advanced-connection-options.tsx | 8 +--- .../advanced-options-tabs.spec.tsx | 44 ++++-------------- .../advanced-options-tabs.tsx | 46 +++++++------------ .../src/components/connect-form.tsx | 3 +- .../src/types/connect-form-tab.ts | 6 --- packages/connect-form/src/utils/validation.ts | 12 +++-- 6 files changed, 35 insertions(+), 84 deletions(-) delete mode 100644 packages/connect-form/src/types/connect-form-tab.ts diff --git a/packages/connect-form/src/components/advanced-connection-options.tsx b/packages/connect-form/src/components/advanced-connection-options.tsx index 9745f3d2943..11e1c8ffbb7 100644 --- a/packages/connect-form/src/components/advanced-connection-options.tsx +++ b/packages/connect-form/src/components/advanced-connection-options.tsx @@ -9,10 +9,7 @@ import { ConnectionOptions } from 'mongodb-data-service'; import AdvancedOptionsTabs from './advanced-options-tabs/advanced-options-tabs'; import { UpdateConnectionFormField } from '../hooks/use-connect-form'; -import { - ConnectionFormError, - ConnectionFormWarning, -} from '../utils/validation'; +import { ConnectionFormError } from '../utils/validation'; const disabledOverlayStyles = css({ position: 'absolute', @@ -35,13 +32,11 @@ function AdvancedConnectionOptions({ errors, updateConnectionFormField, connectionOptions, - warnings, }: { errors: ConnectionFormError[]; disabled: boolean; updateConnectionFormField: UpdateConnectionFormField; connectionOptions: ConnectionOptions; - warnings: ConnectionFormWarning[]; }): React.ReactElement { return ( @@ -56,7 +51,6 @@ function AdvancedConnectionOptions({ errors={errors} updateConnectionFormField={updateConnectionFormField} connectionOptions={connectionOptions} - warnings={warnings} /> diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx index 9d8a68c56ff..1fafa9a8ffa 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx @@ -23,7 +23,6 @@ describe('AdvancedOptionsTabs Component', function () { connectionString: testUrl, }} errors={[]} - warnings={[]} updateConnectionFormField={updateConnectionFormFieldSpy} /> ); @@ -39,7 +38,7 @@ describe('AdvancedOptionsTabs Component', function () { }); }); - it('should have only the tab with an error on it showing an error', function () { + it('should have the tab with an error have the error', function () { render( ); ['General', 'Authentication', 'TLS/SSL', 'Proxy/SSH Tunnel'].forEach( (tabName) => { - expect(screen.queryByTestId(`${tabName}-tab-has-error`)).to.not.exist; - } - ); - expect(screen.getAllByTestId('Advanced-tab-has-error')[0]).to.be.visible; - }); - - it('the tab with a warning should have the warning test id (and hopefully the visual indicator)', function () { - render( - - ); - - ['General', 'TLS/SSL', 'Proxy/SSH Tunnel', 'Advanced'].forEach( - (tabName) => { - expect(screen.queryByTestId(`${tabName}-tab-has-warning`)).to.not.exist; + expect( + screen + .getAllByTestId(`${tabName}-tab`)[0] + .getAttribute('data-has-error') + ).to.equal('false'); } ); - expect(screen.getAllByTestId('Authentication-tab-has-warning')[0]).to.be - .visible; + expect( + screen.getAllByTestId('Advanced-tab')[0].getAttribute('data-has-error') + ).to.equal('true'); }); }); diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index 017b48eaec9..be2c147d946 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -1,5 +1,12 @@ import React, { useState, useMemo } from 'react'; -import { Tabs, Tab, spacing, css, cx } from '@mongodb-js/compass-components'; +import { + Tabs, + Tab, + spacing, + css, + cx, + uiColors, +} from '@mongodb-js/compass-components'; import ConnectionStringUrl from 'mongodb-connection-string-url'; import { ConnectionOptions } from 'mongodb-data-service'; @@ -11,16 +18,15 @@ import AdvancedTab from './advanced-tab/advanced-tab'; import { UpdateConnectionFormField } from '../../hooks/use-connect-form'; import { ConnectionFormError, - ConnectionFormWarning, + ConnectFormTabName, } from '../../utils/validation'; import { defaultConnectionString } from '../../constants/default-connection'; -import { ConnectFormTab } from '../../types/connect-form-tab'; const tabsStyles = css({ marginTop: spacing[1], }); -const tabWithIndicatorStyles = css({ +const tabWithErrorIndicatorStyles = css({ position: 'relative', '[role=tab]&::before': { position: 'absolute', @@ -30,23 +36,12 @@ const tabWithIndicatorStyles = css({ width: spacing[2], height: spacing[2], borderRadius: '50%', - }, -}); - -const tabWithErrorStyle = css({ - '[role=tab]&::before': { - backgroundColor: 'red', - }, -}); - -const tabWithWarningStyle = css({ - 'button &::before': { - backgroundColor: 'yellow', + backgroundColor: uiColors.red.base, }, }); interface TabObject { - name: ConnectFormTab; + name: ConnectFormTabName; component: React.FunctionComponent<{ errors: ConnectionFormError[]; connectionStringUrl: ConnectionStringUrl; @@ -59,12 +54,10 @@ function AdvancedOptionsTabs({ errors, updateConnectionFormField, connectionOptions, - warnings, }: { errors: ConnectionFormError[]; updateConnectionFormField: UpdateConnectionFormField; connectionOptions: ConnectionOptions; - warnings: ConnectionFormWarning[]; }): React.ReactElement { const [activeTab, setActiveTab] = useState(0); @@ -95,27 +88,20 @@ function AdvancedOptionsTabs({ {tabs.map((tabObject: TabObject, idx: number) => { const TabComponent = tabObject.component; - const tabHasError = !!errors.find( + const showTabErrorIndicator = !!errors.find( (error) => error.fieldTab === tabObject.name ); - // If the tab has an error, don't show the warning indicator. - const tabHasWarning = - !tabHasError && - !!warnings.find((warnings) => warnings.fieldTab === tabObject.name); return ( )} Date: Fri, 28 Jan 2022 12:10:19 -0500 Subject: [PATCH 4/7] use id for tab instead of overload name --- .../advanced-options-tabs.spec.tsx | 2 +- .../advanced-options-tabs.tsx | 24 ++++++++-------- .../connect-form/src/utils/validation.spec.ts | 28 +++++++++---------- packages/connect-form/src/utils/validation.ts | 27 ++++++++---------- 4 files changed, 39 insertions(+), 42 deletions(-) diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx index 1fafa9a8ffa..1d38e44294b 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx @@ -46,7 +46,7 @@ describe('AdvancedOptionsTabs Component', function () { }} errors={[ { - fieldTab: 'Advanced', + fieldTab: 'advanced', message: 'oranges', }, ]} diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index be2c147d946..6d9ca6b4a07 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -16,10 +16,7 @@ import ProxyAndSshTunnelTab from './ssh-tunnel-tab/proxy-and-ssh-tunnel-tab'; import TLSTab from './tls-ssl-tab/tls-ssl-tab'; import AdvancedTab from './advanced-tab/advanced-tab'; import { UpdateConnectionFormField } from '../../hooks/use-connect-form'; -import { - ConnectionFormError, - ConnectFormTabName, -} from '../../utils/validation'; +import { ConnectionFormError, TabId } from '../../utils/validation'; import { defaultConnectionString } from '../../constants/default-connection'; const tabsStyles = css({ @@ -41,7 +38,8 @@ const tabWithErrorIndicatorStyles = css({ }); interface TabObject { - name: ConnectFormTabName; + name: string; + id: TabId; component: React.FunctionComponent<{ errors: ConnectionFormError[]; connectionStringUrl: ConnectionStringUrl; @@ -62,11 +60,15 @@ function AdvancedOptionsTabs({ const [activeTab, setActiveTab] = useState(0); const tabs: TabObject[] = [ - { name: 'General', component: GeneralTab }, - { name: 'Authentication', component: AuthenticationTab }, - { name: 'TLS/SSL', component: TLSTab }, - { name: 'Proxy/SSH Tunnel', component: ProxyAndSshTunnelTab }, - { name: 'Advanced', component: AdvancedTab }, + { name: 'General', id: 'general', component: GeneralTab }, + { + name: 'Authentication', + id: 'authentication', + component: AuthenticationTab, + }, + { name: 'TLS/SSL', id: 'tls', component: TLSTab }, + { name: 'Proxy/SSH Tunnel', id: 'proxy', component: ProxyAndSshTunnelTab }, + { name: 'Advanced', id: 'advanced', component: AdvancedTab }, ]; const connectionStringUrl = useMemo(() => { @@ -89,7 +91,7 @@ function AdvancedOptionsTabs({ const TabComponent = tabObject.component; const showTabErrorIndicator = !!errors.find( - (error) => error.fieldTab === tabObject.name + (error) => error.fieldTab === tabObject.id ); return ( diff --git a/packages/connect-form/src/utils/validation.spec.ts b/packages/connect-form/src/utils/validation.spec.ts index 2491ff11c6a..8dfe12fd176 100644 --- a/packages/connect-form/src/utils/validation.spec.ts +++ b/packages/connect-form/src/utils/validation.spec.ts @@ -66,7 +66,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'sshHostname', - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', message: 'A hostname is required to connect with an SSH tunnel.', }, ]); @@ -89,7 +89,7 @@ describe('validation', function () { ); expect(result).to.deep.equal([ { - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', message: 'When connecting via SSH tunnel either password or identity file is required.', }, @@ -111,7 +111,7 @@ describe('validation', function () { ).to.deep.equal([ { fieldName: 'sshIdentityKeyFile', - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', message: 'File is required along with passphrase.', }, ]); @@ -153,7 +153,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'proxyHostname', - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', message: 'Proxy hostname is required.', }, ]); @@ -167,7 +167,7 @@ describe('validation', function () { }); expect(result).to.deep.equal([ { - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'TLS must be enabled in order to use x509 authentication.', }, ]); @@ -178,7 +178,7 @@ describe('validation', function () { }); expect(result).to.deep.equal([ { - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'TLS must be enabled in order to use x509 authentication.', }, ]); @@ -190,7 +190,7 @@ describe('validation', function () { }); expect(result).to.deep.equal([ { - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'TLS must be enabled in order to use x509 authentication.', }, ]); @@ -222,7 +222,7 @@ describe('validation', function () { }); expect(result).to.deep.equal([ { - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'A Client Certificate is required with x509 authentication.', }, @@ -237,7 +237,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'password', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Password is missing.', }, ]); @@ -251,7 +251,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'kerberosPrincipal', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Principal name is required with Kerberos.', }, ]); @@ -271,12 +271,12 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'username', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Username is missing.', }, { fieldName: 'password', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Password is missing.', }, ]); @@ -288,7 +288,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'password', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Password is missing.', }, ]); @@ -314,7 +314,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { fieldName: 'password', - fieldTab: 'Authentication', + fieldTab: 'authentication', message: 'Password is missing.', }, ]); diff --git a/packages/connect-form/src/utils/validation.ts b/packages/connect-form/src/utils/validation.ts index b5c04036110..0bbd7bd7732 100644 --- a/packages/connect-form/src/utils/validation.ts +++ b/packages/connect-form/src/utils/validation.ts @@ -18,17 +18,12 @@ export type FieldName = | 'sshUsername' | 'username'; -export type ConnectFormTabName = - | 'General' - | 'Authentication' - | 'TLS/SSL' - | 'Proxy/SSH Tunnel' - | 'Advanced'; +export type TabId = 'general' | 'authentication' | 'tls' | 'proxy' | 'advanced'; export type ConnectionFormError = { fieldName?: FieldName; fieldIndex?: number; - fieldTab?: ConnectFormTabName; + fieldTab?: TabId; message: string; }; @@ -119,7 +114,7 @@ function validateUsernameAndPasswordErrors( const errors: ConnectionFormError[] = []; if (!username) { errors.push({ - fieldTab: 'Authentication', + fieldTab: 'authentication', fieldName: 'username', message: 'Username is missing.', }); @@ -127,7 +122,7 @@ function validateUsernameAndPasswordErrors( if (!password) { errors.push({ - fieldTab: 'Authentication', + fieldTab: 'authentication', fieldName: 'password', message: 'Password is missing.', }); @@ -140,13 +135,13 @@ function validateX509Errors( const errors: ConnectionFormError[] = []; if (!isSecure(connectionString)) { errors.push({ - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'TLS must be enabled in order to use x509 authentication.', }); } if (!connectionString.searchParams.has('tlsCertificateKeyFile')) { errors.push({ - fieldTab: 'TLS/SSL', + fieldTab: 'tls', message: 'A Client Certificate is required with x509 authentication.', }); } @@ -164,7 +159,7 @@ function validateKerberosErrors( const errors: ConnectionFormError[] = []; if (!connectionString.username) { errors.push({ - fieldTab: 'Authentication', + fieldTab: 'authentication', fieldName: 'kerberosPrincipal', message: 'Principal name is required with Kerberos.', }); @@ -178,7 +173,7 @@ function validateSSHTunnelErrors( const errors: ConnectionFormError[] = []; if (!sshTunnel.host) { errors.push({ - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', fieldName: 'sshHostname', message: 'A hostname is required to connect with an SSH tunnel.', }); @@ -186,7 +181,7 @@ function validateSSHTunnelErrors( if (!sshTunnel.password && !sshTunnel.identityKeyFile) { errors.push({ - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', message: 'When connecting via SSH tunnel either password or identity file is required.', }); @@ -194,7 +189,7 @@ function validateSSHTunnelErrors( if (sshTunnel.identityKeyPassphrase && !sshTunnel.identityKeyFile) { errors.push({ - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', fieldName: 'sshIdentityKeyFile', message: 'File is required along with passphrase.', }); @@ -215,7 +210,7 @@ function validateSocksProxyErrors( const errors: ConnectionFormError[] = []; if (!proxyHost && (proxyPort || proxyUsername || proxyPassword)) { errors.push({ - fieldTab: 'Proxy/SSH Tunnel', + fieldTab: 'proxy', fieldName: 'proxyHostname', message: 'Proxy hostname is required.', }); From 5e905a8ce837a7d7ce909bbe7d78f8f98ffa0f15 Mon Sep 17 00:00:00 2001 From: Anemy Date: Fri, 28 Jan 2022 15:54:53 -0500 Subject: [PATCH 5/7] Add error count to aria-label with test --- .../advanced-options-tabs.spec.tsx | 38 ++++++++++++++++--- .../advanced-options-tabs.tsx | 11 +++++- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx index 1d38e44294b..3a6046c4ca9 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx @@ -56,15 +56,43 @@ describe('AdvancedOptionsTabs Component', function () { ['General', 'Authentication', 'TLS/SSL', 'Proxy/SSH Tunnel'].forEach( (tabName) => { - expect( - screen - .getAllByTestId(`${tabName}-tab`)[0] - .getAttribute('data-has-error') - ).to.equal('false'); + const tab = screen.getAllByTestId(`${tabName}-tab`)[0]; + expect(tab.getAttribute('data-has-error')).to.equal('false'); } ); expect( screen.getAllByTestId('Advanced-tab')[0].getAttribute('data-has-error') ).to.equal('true'); }); + + it('should have an aria-label for the tab that shows the error count', function () { + render( + + ); + + ['General', 'Authentication', 'Proxy/SSH Tunnel', 'Advanced'].forEach( + (tabName) => { + const tab = screen.getAllByTestId(`${tabName}-tab`)[0]; + expect(tab.getAttribute('aria-label')).to.equal(tabName); + } + ); + expect( + screen.getAllByTestId('TLS/SSL-tab')[0].getAttribute('aria-label') + ).to.equal('TLS/SSL (2 errors)'); + }); }); diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index 6d9ca6b4a07..bfea65feb80 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -90,9 +90,10 @@ function AdvancedOptionsTabs({ {tabs.map((tabObject: TabObject, idx: number) => { const TabComponent = tabObject.component; - const showTabErrorIndicator = !!errors.find( + const tabErrors = errors.filter( (error) => error.fieldTab === tabObject.id ); + const showTabErrorIndicator = tabErrors.length > 0; return ( 0 + ? ` (${tabErrors.length} error${ + tabErrors.length > 1 ? 's' : '' + })` + : '' + }`} data-testid={`${tabObject.name}-tab`} data-has-error={showTabErrorIndicator} > From 3fec1aad07827664b40af564d5eb7d1f487c34cf Mon Sep 17 00:00:00 2001 From: Anemy Date: Mon, 31 Jan 2022 10:26:40 -0500 Subject: [PATCH 6/7] better types for errors, add field tab to related errors --- .../advanced-options-tabs.spec.tsx | 3 ++ .../advanced-options-tabs.tsx | 10 ++++--- .../general-tab/host-input.spec.tsx | 1 + .../src/components/connect-form.tsx | 2 +- .../src/hooks/use-connect-form.spec.ts | 4 +++ .../src/hooks/use-connect-form.ts | 3 ++ .../src/utils/authentication-handler.spec.ts | 3 ++ .../src/utils/authentication-handler.ts | 4 +++ .../connect-form/src/utils/validation.spec.ts | 5 ++++ packages/connect-form/src/utils/validation.ts | 28 ++++++++++++++++--- 10 files changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx index 3a6046c4ca9..7d6f5908310 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.spec.tsx @@ -47,6 +47,7 @@ describe('AdvancedOptionsTabs Component', function () { errors={[ { fieldTab: 'advanced', + fieldName: 'connectionString', message: 'oranges', }, ]} @@ -74,10 +75,12 @@ describe('AdvancedOptionsTabs Component', function () { errors={[ { fieldTab: 'tls', + fieldName: 'tls', message: 'oranges', }, { fieldTab: 'tls', + fieldName: 'tlsCertificateKeyFile', message: 'peaches', }, ]} diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index bfea65feb80..69266b3051c 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -16,7 +16,11 @@ import ProxyAndSshTunnelTab from './ssh-tunnel-tab/proxy-and-ssh-tunnel-tab'; import TLSTab from './tls-ssl-tab/tls-ssl-tab'; import AdvancedTab from './advanced-tab/advanced-tab'; import { UpdateConnectionFormField } from '../../hooks/use-connect-form'; -import { ConnectionFormError, TabId } from '../../utils/validation'; +import { + ConnectionFormError, + TabId, + errorsByFieldTab, +} from '../../utils/validation'; import { defaultConnectionString } from '../../constants/default-connection'; const tabsStyles = css({ @@ -90,9 +94,7 @@ function AdvancedOptionsTabs({ {tabs.map((tabObject: TabObject, idx: number) => { const TabComponent = tabObject.component; - const tabErrors = errors.filter( - (error) => error.fieldTab === tabObject.id - ); + const tabErrors = errorsByFieldTab(errors, tabObject.id); const showTabErrorIndicator = tabErrors.length > 0; return ( diff --git a/packages/connect-form/src/components/advanced-options-tabs/general-tab/host-input.spec.tsx b/packages/connect-form/src/components/advanced-options-tabs/general-tab/host-input.spec.tsx index 3ff73c24b88..922210fa6b9 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/general-tab/host-input.spec.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/general-tab/host-input.spec.tsx @@ -88,6 +88,7 @@ describe('HostInput', function () { errors={[ { fieldName: 'hosts', + fieldTab: 'general', fieldIndex: 1, message: 'Eeeee!!!', }, diff --git a/packages/connect-form/src/components/connect-form.tsx b/packages/connect-form/src/components/connect-form.tsx index bf47b01040a..dd5ef37634c 100644 --- a/packages/connect-form/src/components/connect-form.tsx +++ b/packages/connect-form/src/components/connect-form.tsx @@ -176,7 +176,7 @@ function ConnectForm({ )} error.fieldTab === tabId); +} + export function errorMessageByFieldName( errors: ConnectionFormError[], fieldName: FieldName @@ -136,12 +153,14 @@ function validateX509Errors( if (!isSecure(connectionString)) { errors.push({ fieldTab: 'tls', + fieldName: 'tls', message: 'TLS must be enabled in order to use x509 authentication.', }); } if (!connectionString.searchParams.has('tlsCertificateKeyFile')) { errors.push({ fieldTab: 'tls', + fieldName: 'tlsCertificateKeyFile', message: 'A Client Certificate is required with x509 authentication.', }); } @@ -182,6 +201,7 @@ function validateSSHTunnelErrors( if (!sshTunnel.password && !sshTunnel.identityKeyFile) { errors.push({ fieldTab: 'proxy', + fieldName: 'sshPassword', message: 'When connecting via SSH tunnel either password or identity file is required.', }); From bb7ffe4c735becf406af9f2b9d4f3e0e50ba493f Mon Sep 17 00:00:00 2001 From: Anemy Date: Mon, 31 Jan 2022 10:41:15 -0500 Subject: [PATCH 7/7] Use react component prop instead of string for tab name --- .../advanced-options-tabs.tsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx index 69266b3051c..1c270663b35 100644 --- a/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx +++ b/packages/connect-form/src/components/advanced-options-tabs/advanced-options-tabs.tsx @@ -29,10 +29,10 @@ const tabsStyles = css({ const tabWithErrorIndicatorStyles = css({ position: 'relative', - '[role=tab]&::before': { + '&::after': { position: 'absolute', - top: spacing[1], - right: spacing[1], + top: -spacing[2], + right: -spacing[2], content: '""', width: spacing[2], height: spacing[2], @@ -99,11 +99,16 @@ function AdvancedOptionsTabs({ return ( + {tabObject.name} + + } aria-label={`${tabObject.name}${ tabErrors.length > 0 ? ` (${tabErrors.length} error${