From 4f1ae4f19b700e7b0fed89785821972bc75624e9 Mon Sep 17 00:00:00 2001 From: Andy Brown Date: Fri, 10 Apr 2020 10:52:47 -0700 Subject: [PATCH] fix: various form bugs (#2592) * force number field to display blank value when no value set * differentiate number and integer * do not unset value if selecting the same type * remove unecessary fragment * make sure scroll bar appears correctly for form * fix problem editing open object values * remove dead styles * fix ability to edit form title * add missing aria labels * send focus to key field after adding new value * translate error messages * do not default value to string * use simplified margin syntax * extract getValueType into utils Co-authored-by: Chris Whitten --- .../client/src/pages/design/index.tsx | 14 +- .../client/src/pages/design/styles.ts | 3 +- .../src/components/AdaptiveForm/FormTitle.tsx | 2 +- .../src/components/fields/NumberField.tsx | 13 +- .../src/components/fields/OneOfField.tsx | 4 +- .../fields/OpenObjectField/ObjectItem.tsx | 110 +++++++++++++++ .../index.tsx} | 130 ++++-------------- .../fields/OpenObjectField/styles.ts | 45 ++++++ .../src/components/fields/styles.ts | 38 ----- .../src/utils/__tests__/getValueType.test.ts | 43 ++++++ .../adaptive-form/src/utils/getValueType.ts | 19 +++ .../adaptive-form/src/utils/index.ts | 1 + .../expressions/src/ExpressionField.tsx | 10 +- 13 files changed, 273 insertions(+), 159 deletions(-) create mode 100644 Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/ObjectItem.tsx rename Composer/packages/extensions/adaptive-form/src/components/fields/{OpenObjectField.tsx => OpenObjectField/index.tsx} (50%) create mode 100644 Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/styles.ts create mode 100644 Composer/packages/extensions/adaptive-form/src/utils/__tests__/getValueType.test.ts create mode 100644 Composer/packages/extensions/adaptive-form/src/utils/getValueType.ts diff --git a/Composer/packages/client/src/pages/design/index.tsx b/Composer/packages/client/src/pages/design/index.tsx index d5fdac91b9..f483821b1f 100644 --- a/Composer/packages/client/src/pages/design/index.tsx +++ b/Composer/packages/client/src/pages/design/index.tsx @@ -368,15 +368,13 @@ function DesignPage(props) {
{match && } - -
-
- {breadcrumbItems} - -
- +
+
+ {breadcrumbItems} +
- + +
diff --git a/Composer/packages/client/src/pages/design/styles.ts b/Composer/packages/client/src/pages/design/styles.ts index dd35a858a7..8bef12fccb 100644 --- a/Composer/packages/client/src/pages/design/styles.ts +++ b/Composer/packages/client/src/pages/design/styles.ts @@ -86,7 +86,8 @@ export const formEditor = css` flex: 1; border: 0px; transition: width 0.2s ease-in-out; - overflow: auto; + overflow-y: scroll; + height: 100%; `; export const breadcrumbClass = mergeStyleSets({ diff --git a/Composer/packages/extensions/adaptive-form/src/components/AdaptiveForm/FormTitle.tsx b/Composer/packages/extensions/adaptive-form/src/components/AdaptiveForm/FormTitle.tsx index 4e93ccab4c..2c5a76b120 100644 --- a/Composer/packages/extensions/adaptive-form/src/components/AdaptiveForm/FormTitle.tsx +++ b/Composer/packages/extensions/adaptive-form/src/components/AdaptiveForm/FormTitle.tsx @@ -28,7 +28,7 @@ interface FormTitleProps { const FormTitle: React.FC = props => { const { name, description, schema, formData, uiOptions = {} } = props; - const handleTitleChange = (_e: React.FormEvent, newTitle?: string): void => { + const handleTitleChange = (newTitle?: string): void => { if (props.onChange) { props.onChange({ ...formData.$designer, diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/NumberField.tsx b/Composer/packages/extensions/adaptive-form/src/components/fields/NumberField.tsx index 6e8e59ea17..14bdf175fb 100644 --- a/Composer/packages/extensions/adaptive-form/src/components/fields/NumberField.tsx +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/NumberField.tsx @@ -17,12 +17,17 @@ const getFloat = (value: string, step: number) => { return parseFloat(fixed); }; -export const NumberField: React.FC = function NumberField(props) { +const NumberField: React.FC = props => { const { description, disabled, id, label, onChange, readonly, schema, value, uiOptions } = props; const { type } = schema; const updateValue = (step: number) => (value: string) => { + if (value === '') { + onChange(0); + return; + } + // if the number is a float, we need to convert to a fixed decimal place // in order to avoid floating point math rounding errors (ex. 1.2000000001) // ex. if step = 0.01, we fix to 2 decimals @@ -32,17 +37,19 @@ export const NumberField: React.FC = function NumberField(props) { }; const step = type === 'integer' ? 1 : 0.1; + const displayValue = typeof value === 'number' ? value.toString() : ''; return ( <> = function NumberField(props) { ); }; + +export { NumberField }; diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/OneOfField.tsx b/Composer/packages/extensions/adaptive-form/src/components/fields/OneOfField.tsx index 1f20681e5e..ae3ce152f3 100644 --- a/Composer/packages/extensions/adaptive-form/src/components/fields/OneOfField.tsx +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/OneOfField.tsx @@ -9,7 +9,7 @@ import { Dropdown, IDropdownOption, ResponsiveMode } from 'office-ui-fabric-reac import formatMessage from 'format-message'; import { FieldLabel } from '../FieldLabel'; -import { resolveRef, resolveFieldWidget } from '../../utils'; +import { resolveRef, resolveFieldWidget, getValueType } from '../../utils'; import { usePluginConfig } from '../../hooks'; import { oneOfField } from './styles'; @@ -53,7 +53,7 @@ const getSelectedOption = (value: any | undefined, options: IDropdownOption[]): return; } - const valueType = Array.isArray(value) ? 'array' : typeof value; + const valueType = getValueType(value); if (valueType === 'array') { const item = value[0]; diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/ObjectItem.tsx b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/ObjectItem.tsx new file mode 100644 index 0000000000..205179023d --- /dev/null +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/ObjectItem.tsx @@ -0,0 +1,110 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** @jsx jsx */ +import { jsx } from '@emotion/core'; +import React, { useState, useCallback, useMemo } from 'react'; +import { FontSizes, NeutralColors } from '@uifabric/fluent-theme'; +import { IconButton } from 'office-ui-fabric-react/lib/Button'; +import { IContextualMenuItem } from 'office-ui-fabric-react/lib/ContextualMenu'; +import formatMessage from 'format-message'; + +import { EditableField } from '../EditableField'; + +import { container, item } from './styles'; + +interface ObjectItemProps { + name: string; + formData: any; + value: any; + onNameChange: (name: string) => void; + onValueChange: (value?: string) => void; + onDelete: () => void; +} + +const ObjectItem: React.FC = ({ + name: originalName, + formData, + value, + onNameChange, + onValueChange, + onDelete, +}) => { + const initialName = useMemo(() => originalName, []); + const initialValue = useMemo(() => value, []); + const [name, setName] = useState(originalName); + const [errorMessage, setErrorMessage] = useState(''); + + const contextItems: IContextualMenuItem[] = [ + { + iconProps: { iconName: 'Cancel' }, + key: 'remove', + onClick: onDelete, + text: 'Remove', + }, + ]; + + const handleBlur = useCallback(() => { + if (!name || name === '') { + setErrorMessage(formatMessage('Key cannot be blank')); + } else if (name !== originalName && Object.keys(formData).includes(name)) { + setErrorMessage(formatMessage('Keys must be unique')); + } else { + onNameChange(name); + setErrorMessage(''); + } + }, [name, formData]); + + return ( +
+
+ setName(newValue || '')} + ariaLabel={formatMessage('key')} + /> +
+
+ +
+ +
+ ); +}; + +export { ObjectItem }; diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField.tsx b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/index.tsx similarity index 50% rename from Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField.tsx rename to Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/index.tsx index e62fb91c31..4faa4ceed8 100644 --- a/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField.tsx +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/index.tsx @@ -2,101 +2,19 @@ // Licensed under the MIT License. /** @jsx jsx */ import { jsx } from '@emotion/core'; -import React, { useState } from 'react'; +import React, { useState, useRef } from 'react'; import { FontSizes, NeutralColors, SharedColors } from '@uifabric/fluent-theme'; import { IconButton } from 'office-ui-fabric-react/lib/Button'; -import { IContextualMenuItem } from 'office-ui-fabric-react/lib/ContextualMenu'; -import { TextField } from 'office-ui-fabric-react/lib/TextField'; +import { TextField, ITextField } from 'office-ui-fabric-react/lib/TextField'; import { FieldProps } from '@bfc/extension'; import formatMessage from 'format-message'; -import { FieldLabel } from '../FieldLabel'; +import { FieldLabel } from '../../FieldLabel'; -import { openObjectField } from './styles'; -import { EditableField } from './EditableField'; +import * as styles from './styles'; +import { ObjectItem } from './ObjectItem'; -const ObjectItem = ({ - name: originalName, - formData, - value, - handleNameChange, - handleValueChange, - handleDropPropertyClick, -}) => { - const [name, setName] = useState(originalName); - const [errorMessage, setErrorMessage] = useState(''); - - const contextItems: IContextualMenuItem[] = [ - { - iconProps: { iconName: 'Cancel' }, - key: 'remove', - onClick: handleDropPropertyClick, - text: 'Remove', - }, - ]; - - const handleBlur = () => { - if (name !== originalName && Object.keys(formData).includes(name)) { - setErrorMessage(formatMessage('Keys must be unique')); - } else { - handleNameChange(name); - setErrorMessage(''); - } - }; - - return ( -
-
- setName(newValue || '')} - ariaLabel={formatMessage('key')} - /> -
-
- -
- -
- ); -}; - -export const OpenObjectField: React.FC> = props => { const { @@ -111,6 +29,7 @@ export const OpenObjectField: React.FC(''); const [newValue, setNewValue] = useState(''); + const fieldRef = useRef(null); const handleKeyDown = event => { if (event.key.toLowerCase() === 'enter') { @@ -120,6 +39,10 @@ export const OpenObjectField: React.FC (_, newValue) => { + const handleValueChange = (name: string) => (newValue?: string) => { onChange({ ...value, [name]: newValue || '' }); }; @@ -143,50 +66,51 @@ export const OpenObjectField: React.FC -
-
+
+
-
+
-
+
{Object.entries(value).map(([name, value], index) => { return ( ); })} {additionalProperties && ( -
-
+
+
setName(newValue || '')} onKeyDown={handleKeyDown} + componentRef={fieldRef} />
-
+
); }; + +export { OpenObjectField }; diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/styles.ts b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/styles.ts new file mode 100644 index 0000000000..d19a6311a5 --- /dev/null +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/OpenObjectField/styles.ts @@ -0,0 +1,45 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { css } from '@emotion/core'; +import { NeutralColors } from '@uifabric/fluent-theme'; + +export const container = css` + border-top: 1px solid ${NeutralColors.gray30}; + display: flex; + + label: OpenObjectFieldContainer; +`; + +export const filler = css` + width: 32px; + + label: OpenObjectFieldFiller; +`; + +export const item = css` + flex: 1; + + & + & { + margin-left: 16px; + } + + label: OpenObjectFieldItem; +`; + +export const label = css` + flex: 1; + padding-left: 8px; + + & + & { + margin-left: 16px; + } + + label: OpenObjectFieldLabel; +`; + +export const labelContainer = css` + display: flex; + + label: OpenObjectFieldLabelContainer; +`; diff --git a/Composer/packages/extensions/adaptive-form/src/components/fields/styles.ts b/Composer/packages/extensions/adaptive-form/src/components/fields/styles.ts index a4969ef893..33fe170d3f 100644 --- a/Composer/packages/extensions/adaptive-form/src/components/fields/styles.ts +++ b/Composer/packages/extensions/adaptive-form/src/components/fields/styles.ts @@ -83,44 +83,6 @@ export const unsupportedField = { `, }; -export const openObjectField = { - container: css` - border-top: 1px solid ${NeutralColors.gray30}; - display: flex; - - label: OpenObjectFieldContainer; - `, - filler: css` - width: 32px; - - label: OpenObjectFieldFiller; - `, - item: css` - flex: 1; - - & + & { - margin-left: 16px; - } - - label: OpenObjectFieldItem; - `, - label: css` - flex: 1; - padding-left: 8px; - - & + & { - margin-left: 16px; - } - - label: OpenObjectFieldLabel; - `, - labelContainer: css` - display: flex; - - label: OpenObjectFieldLabelContainer; - `, -}; - export const objectArrayField = { objectItemLabel: css` display: flex; diff --git a/Composer/packages/extensions/adaptive-form/src/utils/__tests__/getValueType.test.ts b/Composer/packages/extensions/adaptive-form/src/utils/__tests__/getValueType.test.ts new file mode 100644 index 0000000000..8d748de885 --- /dev/null +++ b/Composer/packages/extensions/adaptive-form/src/utils/__tests__/getValueType.test.ts @@ -0,0 +1,43 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { getValueType } from '../getValueType'; + +describe('getValueType', () => { + it('returns `array` for array types', () => { + expect(getValueType([])).toEqual('array'); + }); + + it('returns `integer` for integers', () => { + expect(getValueType(123)).toEqual('integer'); + }); + + it('returns `number` for floats', () => { + expect(getValueType(1.23)).toEqual('number'); + }); + + it('returns js type for other types', () => { + const tests = [ + { + value: 'string', + type: 'string', + }, + { + value: {}, + type: 'object', + }, + { + value: true, + type: 'boolean', + }, + { + value: undefined, + type: 'undefined', + }, + ]; + + tests.forEach(t => { + expect(getValueType(t.value)).toEqual(t.type); + }); + }); +}); diff --git a/Composer/packages/extensions/adaptive-form/src/utils/getValueType.ts b/Composer/packages/extensions/adaptive-form/src/utils/getValueType.ts new file mode 100644 index 0000000000..83a75e9726 --- /dev/null +++ b/Composer/packages/extensions/adaptive-form/src/utils/getValueType.ts @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import isNumber from 'lodash/isNumber'; + +/** + * Returns JSON Schema type for given value. + */ +export function getValueType(value: unknown) { + if (Array.isArray(value)) { + return 'array'; + } + + if (isNumber(value)) { + return Number.isInteger(value) ? 'integer' : 'number'; + } + + return typeof value; +} diff --git a/Composer/packages/extensions/adaptive-form/src/utils/index.ts b/Composer/packages/extensions/adaptive-form/src/utils/index.ts index d8e72d729b..d2bad4fa73 100644 --- a/Composer/packages/extensions/adaptive-form/src/utils/index.ts +++ b/Composer/packages/extensions/adaptive-form/src/utils/index.ts @@ -4,6 +4,7 @@ export * from './arrayUtils'; export * from './getAdaptiveType'; export * from './getOrderedProperties'; export * from './getUISchema'; +export * from './getValueType'; export * from './mergePluginConfigs'; export * from './resolveBaseSchema'; export * from './resolveFieldWidget'; diff --git a/Composer/packages/ui-plugins/expressions/src/ExpressionField.tsx b/Composer/packages/ui-plugins/expressions/src/ExpressionField.tsx index 17b5a6a1ef..d091590310 100644 --- a/Composer/packages/ui-plugins/expressions/src/ExpressionField.tsx +++ b/Composer/packages/ui-plugins/expressions/src/ExpressionField.tsx @@ -5,7 +5,7 @@ import { jsx, css } from '@emotion/core'; import React, { useMemo, useState } from 'react'; import { FieldProps, JSONSchema7 } from '@bfc/extension'; -import { FieldLabel, resolveRef, resolveFieldWidget, usePluginConfig } from '@bfc/adaptive-form'; +import { FieldLabel, resolveRef, resolveFieldWidget, usePluginConfig, getValueType } from '@bfc/adaptive-form'; import { Dropdown, IDropdownOption, ResponsiveMode } from 'office-ui-fabric-react/lib/Dropdown'; import { JsonEditor } from '@bfc/code-editor'; import formatMessage from 'format-message'; @@ -81,7 +81,7 @@ const getOptions = (schema: JSONSchema7, definitions): IDropdownOption[] => { const getSelectedOption = (value: any | undefined, options: IDropdownOption[]): IDropdownOption | undefined => { const expressionOption = options.find(({ key }) => key === 'expression'); - const valueType = Array.isArray(value) ? 'array' : typeof value; + const valueType = getValueType(value); // if its an array, we know it's not an expression if (valueType === 'array') { @@ -107,7 +107,7 @@ const getSelectedOption = (value: any | undefined, options: IDropdownOption[]): } // if the value if undefined, either default to expression or the first option - if (!value) { + if (typeof value === 'undefined' || value === null) { return options.length > 2 ? expressionOption : options[0]; // else if the value is a string and starts with '=' it is an expression } else if ( @@ -123,7 +123,7 @@ const getSelectedOption = (value: any | undefined, options: IDropdownOption[]): }; const ExpressionField: React.FC = props => { - const { id, value = '', label, description, schema, uiOptions, definitions } = props; + const { id, value, label, description, schema, uiOptions, definitions } = props; const { $role, ...expressionSchema } = schema; const pluginConfig = usePluginConfig(); @@ -142,7 +142,7 @@ const ExpressionField: React.FC = props => { ] = useState(initialSelectedOption); const handleTypeChange = (_e: React.FormEvent, option?: IDropdownOption) => { - if (option) { + if (option && option.key !== selectedKey) { setSelectedOption(option); props.onChange(undefined); }