use new validation component

superset-frontend/cypress-base/cypress/integration/database/modal.test.ts

@@ -88,4 +88,28 @@ describe('Add database', () => {
     // cy.wait(1000); // wait for potential (incorrect) closing annimation
     // cy.get('[data-test="database-modal"]').should('be.visible');
   });
+
+  it('should close modal after save', () => {
+    cy.get('[data-test="btn-create-database"]').click();
+
+    // type values
+    cy.get('[data-test="database-modal"] input[name="database_name"]')
+      .focus()
+      .type('cypress');
+    cy.get('[data-test="database-modal"] input[name="sqlalchemy_uri"]')
+      .focus()
+      .type('gsheets://');
+
+    // click save
+    cy.get('[data-test="modal-confirm-button"]:not(:disabled)').click();
+
+    // should show error alerts and keep modal open
+    cy.get('.toast').contains('error');
+    cy.wait(1000); // wait for potential (incorrect) closing annimation
+    cy.get('[data-test="database-modal"]').should('be.visible');
+
+    // should be able to close modal
+    cy.get('[data-test="modal-cancel-button"]').click();
+    cy.get('[data-test="database-modal"]').should('not.be.visible');
+  });
 });

superset-frontend/src/components/Form/FormItem.tsx

@@ -41,6 +41,10 @@ const StyledItem = styled(Form.Item)`
         }
       }
     }
+    .ant-form-item-explain {
+      color: ${theme.colors.grayscale.light1};
+      font-size: ${theme.typography.sizes.s - 1}px;
+    }
   `}
 `;
 

superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx

@@ -25,17 +25,18 @@ import FormLabel from './FormLabel';
 export interface LabeledErrorBoundInputProps {
   label?: string;
   validationMethods:
-    | { onBlur: (value: any) => string }
-    | { onChange: (value: any) => string };
+    | { onBlur: (value: any) => void }
+    | { onChange: (value: any) => void };
   errorMessage: string | null;
   helpText?: string;
   required?: boolean;
   id?: string;
+  classname?: string;
   [x: string]: any;
 }
 
 const StyledInput = styled(Input)`
-  margin: 8px 0;
+  margin: ${({ theme }) => `${theme.gridUnit}px 0 ${theme.gridUnit * 2}px`};
 `;
 
 const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
@@ -60,6 +61,12 @@ const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
       }
     }`}
 `;
+const StyledFormGroup = styled('div')`
+  margin-bottom: ${({ theme }) => theme.gridUnit * 5}px;
+  .ant-form-item {
+    margin-bottom: 0;
+  }
+`;
 
 const LabeledErrorBoundInput = ({
   label,
@@ -68,9 +75,10 @@ const LabeledErrorBoundInput = ({
   helpText,
   required = false,
   id,
+  className,
   ...props
 }: LabeledErrorBoundInputProps) => (
-  <>
+  <StyledFormGroup className={className}>
     <FormLabel htmlFor={id} required={required}>
       {label}
     </FormLabel>
@@ -83,7 +91,7 @@ const LabeledErrorBoundInput = ({
     >
       <StyledInput {...props} {...validationMethods} />
     </FormItem>
-  </>
+  </StyledFormGroup>
 );
 
 export default LabeledErrorBoundInput;

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

@@ -17,11 +17,14 @@
  * under the License.
  */
 import React, { FormEvent } from 'react';
-import cx from 'classnames';
+import { SupersetTheme, JsonObject } from '@superset-ui/core';
 import { InputProps } from 'antd/lib/input';
-import { FormLabel, FormItem } from 'src/components/Form';
-import { Input } from 'src/common/components';
-import { StyledFormHeader, formScrollableStyles } from './styles';
+import ValidatedInput from 'src/components/Form/LabeledErrorBoundInput';
+import {
+  StyledFormHeader,
+  formScrollableStyles,
+  validatedFormStyles,
+} from './styles';
 import { DatabaseForm } from '../types';
 
 export const FormFieldOrder = [
@@ -33,64 +36,137 @@ export const FormFieldOrder = [
   'database_name',
 ];
 
-const CHANGE_METHOD = {
-  onChange: 'onChange',
-  onPropertiesChange: 'onPropertiesChange',
-};
+interface FieldPropTypes {
+  required: boolean;
+  changeMethods: { onParametersChange: (value: any) => string } & {
+    onChange: (value: any) => string;
+  };
+  validationErrors: JsonObject | null;
+  getValidation: () => void;
+}
+
+const hostField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="host"
+    name="host"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.host}
+    placeholder="e.g. 127.0.0.1"
+    className="form-group-w-50"
+    label="Host"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const portField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="port"
+    name="port"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.port}
+    placeholder="e.g. 5432"
+    className="form-group-w-50"
+    label="Port"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const databaseField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="database"
+    name="database"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.database}
+    placeholder="e.g. world_population"
+    label="Database name"
+    onChange={changeMethods.onParametersChange}
+    helpText="Copy the name of the PostgreSQL database you are trying to connect to."
+  />
+);
+const usernameField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="username"
+    name="username"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.username}
+    placeholder="e.g. Analytics"
+    label="Username"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const passwordField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="password"
+    name="password"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.password}
+    placeholder="e.g. ********"
+    label="Password"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const displayField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="database_name"
+    name="database_name"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.database_name}
+    placeholder=""
+    label="Display Name"
+    onChange={changeMethods.onChange}
+    helpText="Pick a nickname for this database to display as in Superset."
+  />
+);
 
 const FORM_FIELD_MAP = {
-  host: {
-    description: 'Host',
-    type: 'text',
-    className: 'w-50',
-    placeholder: 'e.g. 127.0.0.1',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
-  port: {
-    description: 'Port',
-    type: 'text',
-    className: 'w-50',
-    placeholder: 'e.g. 5432',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
-  database: {
-    description: 'Database name',
-    type: 'text',
-    label:
-      'Copy the name of the PostgreSQL database you are trying to connect to.',
-    placeholder: 'e.g. world_population',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
-  username: {
-    description: 'Username',
-    type: 'text',
-    placeholder: 'e.g. Analytics',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
-  password: {
-    description: 'Password',
-    type: 'text',
-    placeholder: 'e.g. ********',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
-  database_name: {
-    description: 'Display Name',
-    type: 'text',
-    label: 'Pick a nickname for this database to display as in Superset.',
-    changeMethod: CHANGE_METHOD.onChange,
-  },
-  query: {
-    additionalProperties: {},
-    description: 'Additional parameters',
-    type: 'object',
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
-  },
+  host: hostField,
+  port: portField,
+  database: databaseField,
+  username: usernameField,
+  password: passwordField,
+  database_name: displayField,
 };
 
 const DatabaseConnectionForm = ({
   dbModel: { name, parameters },
   onParametersChange,
   onChange,
+  validationErrors,
+  getValidation,
 }: {
   dbModel: DatabaseForm;
   onParametersChange: (
@@ -99,6 +175,8 @@ const DatabaseConnectionForm = ({
   onChange: (
     event: FormEvent<InputProps> | { target: HTMLInputElement },
   ) => void;
+  validationErrors: JsonObject | null;
+  getValidation: () => void;
 }) => (
   <>
     <StyledFormHeader>
@@ -107,52 +185,30 @@ const DatabaseConnectionForm = ({
         Need help? Learn more about connecting to {name}.
       </p>
     </StyledFormHeader>
-    <div css={formScrollableStyles}>
+    <div
+      // @ts-ignore
+      css={(theme: SupersetTheme) => [
+        formScrollableStyles,
+        validatedFormStyles(theme),
+      ]}
+    >
       {parameters &&
         FormFieldOrder.filter(
           (key: string) =>
             Object.keys(parameters.properties).includes(key) ||
             key === 'database_name',
-        ).map(field => {
-          const {
-            className,
-            description,
-            type,
-            placeholder,
-            label,
-            changeMethod,
-          } = FORM_FIELD_MAP[field];
-          const onEdit =
-            changeMethod === CHANGE_METHOD.onChange
-              ? onChange
-              : onParametersChange;
-          return (
-            <FormItem
-              className={cx(className, `form-group-${className}`)}
-              key={field}
-            >
-              <FormLabel
-                htmlFor={field}
-                required={parameters.required.includes(field)}
-              >
-                {description}
-              </FormLabel>
-              <Input
-                name={field}
-                type={type}
-                id={field}
-                autoComplete="off"
-                placeholder={placeholder}
-                onChange={onEdit}
-              />
-              <p className="helper">{label}</p>
-            </FormItem>
-          );
-        })}
+        ).map(field =>
+          FORM_FIELD_MAP[field]({
+            required: parameters.required.includes(field),
+            changeMethods: { onParametersChange, onChange },
+            validationErrors,
+            getValidation,
+            key: field,
+          }),
+        )}
     </div>
   </>
 );
-
 export const FormFieldMap = FORM_FIELD_MAP;
 
 export default DatabaseConnectionForm;

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx

@@ -336,7 +336,7 @@ describe('DatabaseModal', () => {
         await screen.findByText(/display name/i);
 
         // it does not fetch any databases if no id is passed in
-        expect(fetchMock.calls().length).toEqual(0);
+        expect(fetchMock.calls(DATABASE_FETCH_ENDPOINT).length).toEqual(0);
 
         // todo we haven't hooked this up to load dynamically yet so
         // we can't currently test it