From 131aa705d5fda34a19de3ec9d0420f8cd94d1aa7 Mon Sep 17 00:00:00 2001 From: Andrey Yamanov Date: Fri, 6 Dec 2024 20:41:15 +0100 Subject: [PATCH 01/11] fix(Form): internal logic fixes --- .changeset/wild-parents-suffer.md | 5 ++ src/components/content/Tag/Tag.tsx | 2 +- .../fields/ComboBox/ComboBox.stories.tsx | 23 +++++++++ .../form/Form/use-field/use-field.ts | 48 +++++-------------- src/tokens.ts | 1 + 5 files changed, 41 insertions(+), 38 deletions(-) create mode 100644 .changeset/wild-parents-suffer.md diff --git a/.changeset/wild-parents-suffer.md b/.changeset/wild-parents-suffer.md new file mode 100644 index 000000000..7c7f847f8 --- /dev/null +++ b/.changeset/wild-parents-suffer.md @@ -0,0 +1,5 @@ +--- +'@cube-dev/ui-kit': minor +--- + +Form logic internal fixes. diff --git a/src/components/content/Tag/Tag.tsx b/src/components/content/Tag/Tag.tsx index 7476070a5..c40fae10c 100644 --- a/src/components/content/Tag/Tag.tsx +++ b/src/components/content/Tag/Tag.tsx @@ -34,7 +34,7 @@ const TagElement = tasty({ closable: '0 (2.5x - 1bw) 0 (1x - 1bw)', }, fill: { - '': '#dark.04', + '': '#light', ...Object.keys(THEMES).reduce((map, type) => { map[`[data-type="${type}"]`] = THEMES[type].fill; diff --git a/src/components/fields/ComboBox/ComboBox.stories.tsx b/src/components/fields/ComboBox/ComboBox.stories.tsx index faa2069e5..3c7f350c7 100644 --- a/src/components/fields/ComboBox/ComboBox.stories.tsx +++ b/src/components/fields/ComboBox/ComboBox.stories.tsx @@ -4,6 +4,7 @@ import { userEvent, within } from '@storybook/test'; import { SELECTED_KEY_ARG } from '../../../stories/FormFieldArgs'; import { baseProps } from '../../../stories/lists/baseProps'; +import { Form, useForm } from '../../form/index'; import { ComboBox, CubeComboBoxProps } from './ComboBox'; @@ -32,6 +33,26 @@ const Template: StoryFn> = ( ); +const FormTemplate: StoryFn> = ( + args: CubeComboBoxProps, +) => { + const [form] = useForm(); + + return ( +
+ + Red + Orange + Yellow + Green + Blue + Purple + Violet + +
+ ); +}; + export const Default = Template.bind({}); Default.args = {}; @@ -101,3 +122,5 @@ With1LongOptionFiltered.play = async ({ canvasElement }) => { await userEvent.type(combobox, 'Red'); }; + +export const WithinForm = FormTemplate.bind({}); diff --git a/src/components/form/Form/use-field/use-field.ts b/src/components/form/Form/use-field/use-field.ts index 2edd964f4..f138d7841 100644 --- a/src/components/form/Form/use-field/use-field.ts +++ b/src/components/form/Form/use-field/use-field.ts @@ -1,7 +1,7 @@ import { useEffect, useMemo, useState } from 'react'; import { ValidateTrigger } from '../../../../shared/index'; -import { useEvent, useIsFirstRender } from '../../../../_internal/index'; +import { useEvent } from '../../../../_internal/index'; import { useFormProps } from '../Form'; import { FieldTypes } from '../types'; import { delayValidationRule } from '../validation'; @@ -47,7 +47,6 @@ export function useField>( props = useFormProps(props); let { - defaultValue, id, idPrefix, name, @@ -60,15 +59,9 @@ export function useField>( showValid, shouldUpdate, } = props; - - if (rules && rules.length && validationDelay) { - rules.unshift(delayValidationRule(validationDelay)); - } - const nonInput = !name; const fieldName: string = name != null ? name : ''; - const isFirstRender = useIsFirstRender(); let [fieldId, setFieldId] = useState( id || (idPrefix ? `${idPrefix}_${fieldName}` : fieldName), ); @@ -95,41 +88,22 @@ export function useField>( let field = form?.getFieldInstance(fieldName); - if (field) { - field.rules = rules; - } - - let isRequired = rules && !!rules.find((rule) => rule.required); - - useEffect(() => { - if (!form) return; - - if (field) { - form.forceReRender(); - } else { - form.createField(fieldName); - } - }, [field]); - if (form) { - if (isFirstRender) { - if (!field) { - field = form.createField(fieldName, true); - } - - if (field?.value == null && defaultValue != null) { - form.setFieldValue(fieldName, defaultValue, false, true); - form.updateInitialFieldsValue({ [fieldName]: defaultValue }); - - field = form?.getFieldInstance(fieldName); - } + if (!field) { + field = form.createField(fieldName, true); } + } + + if (field && field.rules !== rules) { + field.rules = rules; - if (!field?.touched && defaultValue != null) { - form.setFieldValue(fieldName, defaultValue, false, true); + if (field.rules && field.rules.length && validationDelay) { + field.rules.unshift(delayValidationRule(validationDelay)); } } + let isRequired = rules && !!rules.find((rule) => rule.required); + const onChangeHandler = useEvent((val: any, dontTouch: boolean) => { if (!form) return; diff --git a/src/tokens.ts b/src/tokens.ts index 492e675e8..8c17f4686 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -48,6 +48,7 @@ const TOKENS = { transition: '80ms', 'clear-color': 'transparent', 'border-color': color('dark', 0.1), + 'border-opaque-color': 'rgb(227, 227, 233)', 'shadow-color': color('dark-03', 0.1), 'draft-color': color('dark', 0.2), 'minor-color': color('dark', 0.65), From 06982236d217e0c7e9b2ab8e8be14d2ae091bd76 Mon Sep 17 00:00:00 2001 From: Andrey Yamanov Date: Sat, 7 Dec 2024 12:13:12 +0100 Subject: [PATCH 02/11] fix(Form): internal logic fixes * 2 --- src/components/fields/RadioGroup/Radio.tsx | 2 + .../form/Form/ComplexForm.stories.tsx | 14 ++--- src/components/form/Form/field.test.tsx | 58 +++++++++---------- .../form/Form/use-field/use-field.ts | 32 +++++++--- 4 files changed, 61 insertions(+), 45 deletions(-) diff --git a/src/components/fields/RadioGroup/Radio.tsx b/src/components/fields/RadioGroup/Radio.tsx index dc18c3750..9661ec691 100644 --- a/src/components/fields/RadioGroup/Radio.tsx +++ b/src/components/fields/RadioGroup/Radio.tsx @@ -162,6 +162,8 @@ export interface CubeRadioProps inputStyles?: Styles; /* The visual type of the radio button */ type?: 'button' | 'radio'; + value?: string; + onChange?: (value: string) => void; } function Radio(props: CubeRadioProps, ref) { diff --git a/src/components/form/Form/ComplexForm.stories.tsx b/src/components/form/Form/ComplexForm.stories.tsx index 4d2e87c62..14e3e67cd 100644 --- a/src/components/form/Form/ComplexForm.stories.tsx +++ b/src/components/form/Form/ComplexForm.stories.tsx @@ -259,8 +259,11 @@ const Template: StoryFn = (args) => { Test - = (args) => { }, ]} necessityIndicator={'label'} - defaultValue="tenphi@gmail.com" - shouldUpdate={({ email }) => { - return !!email; - }} - > - - + shouldUpdate={(prevValues, { email }) => !!email} + /> diff --git a/src/components/form/Form/field.test.tsx b/src/components/form/Form/field.test.tsx index c07ef2c4d..90fa4306e 100644 --- a/src/components/form/Form/field.test.tsx +++ b/src/components/form/Form/field.test.tsx @@ -9,10 +9,10 @@ import { Field } from './Field'; jest.mock('../../../_internal/hooks/use-warn'); describe('Legacy ', () => { - it('should set default value as value', () => { + it.skip('should set default value as value', () => { const { getByRole, formInstance } = renderWithForm( - + , ); @@ -22,7 +22,7 @@ describe('Legacy ', () => { expect(formInstance.getFieldValue('test')).toBe('Hello, World!'); }); - it('should update default value', () => { + it.skip('should update default value', () => { const { rerender, formInstance } = renderWithForm( @@ -40,7 +40,7 @@ describe('Legacy ', () => { expect(formInstance.getFieldValue('test')).toBe('World!'); }); - it('should not update default value if field is touched', async () => { + it.skip('should not update default value if field is touched', async () => { const { rerender, formInstance, getByRole } = renderWithForm( @@ -51,10 +51,8 @@ describe('Legacy ', () => { const input = getByRole('textbox'); - await act(async () => { - await userEvent.clear(input); - await userEvent.type(input, 'Test!'); - }); + await userEvent.clear(input); + await userEvent.type(input, 'Test!'); rerender( @@ -74,10 +72,8 @@ describe('Legacy ', () => { const input = getByRole('textbox'); - await act(async () => { - await userEvent.clear(input); - await userEvent.type(input, 'Hello!'); - }); + await userEvent.clear(input); + await userEvent.type(input, 'Hello!'); rerender( @@ -90,16 +86,21 @@ describe('Legacy ', () => { it('should change value', async () => { const { getByRole, formInstance } = renderWithForm( - + , + { + formProps: { + defaultValues: { + test: 'Hello', + }, + }, + }, ); const input = getByRole('textbox'); - await act(async () => { - await userEvent.type(input, ', World!'); - }); + await userEvent.type(input, ', World!'); expect(input).toHaveValue('Hello, World!'); expect(formInstance.getFieldValue('test')).toBe('Hello, World!'); @@ -147,9 +148,7 @@ describe('Legacy ', () => { expect(cliRadio).toBeChecked(); - await act(async () => { - await userEvent.click(gitRadio); - }); + await userEvent.click(gitRadio); expect(gitRadio).toBeChecked(); }); @@ -159,27 +158,26 @@ describe('Legacy ', () => { const [deployMode] = useState('git'); return ( - + Deploy with CLI Deploy with GIT ); } - const { getByRole, formInstance } = renderWithForm(); + const { getByRole, formInstance } = renderWithForm(, { + formProps: { + defaultValues: { + test: 'cli', + }, + }, + }); const cliRadio = getByRole('radio', { name: 'Deploy with CLI' }); const gitRadio = getByRole('radio', { name: 'Deploy with GIT' }); expect(cliRadio).toBeChecked(); - await act(async () => { - await userEvent.click(gitRadio); - }); + await userEvent.click(gitRadio); expect(formInstance.getFieldValue('test')).toBe('git'); expect(gitRadio).toBeChecked(); @@ -197,7 +195,7 @@ describe('Legacy ', () => { const cliRadio = getByRole('radio', { name: 'Deploy with CLI' }); - await act(async () => await userEvent.click(cliRadio)); + await userEvent.click(cliRadio); expect(formInstance.getFieldValue('test')).toBe('cli'); expect(onChange).toHaveBeenCalled(); diff --git a/src/components/form/Form/use-field/use-field.ts b/src/components/form/Form/use-field/use-field.ts index f138d7841..9f9f746c3 100644 --- a/src/components/form/Form/use-field/use-field.ts +++ b/src/components/form/Form/use-field/use-field.ts @@ -1,7 +1,7 @@ import { useEffect, useMemo, useState } from 'react'; import { ValidateTrigger } from '../../../../shared/index'; -import { useEvent } from '../../../../_internal/index'; +import { useEvent, useWarn } from '../../../../_internal/index'; import { useFormProps } from '../Form'; import { FieldTypes } from '../types'; import { delayValidationRule } from '../validation'; @@ -36,6 +36,12 @@ function removeId(name, id) { ID_MAP[name] = ID_MAP[name].filter((_id) => _id !== id); } +const UNCONTROLLED_FIELDS = [ + 'defaultValue', + 'defaultSelectedKey', + 'defaultSelected', +]; + export type UseFieldParams = { defaultValidationTrigger?: ValidateTrigger; }; @@ -66,6 +72,17 @@ export function useField>( id || (idPrefix ? `${idPrefix}_${fieldName}` : fieldName), ); + const uncontrolledKey = Object.keys(props).find((key) => + UNCONTROLLED_FIELDS.includes(key), + ); + + useWarn(uncontrolledKey && fieldName, { + key: uncontrolledKey, + args: [ + "It's not allowed to use field in uncontrolled mode if it's connected to the form. Use 'defaultValues' prop on Form component to set the default value for each field. You can also disconnect the input from the form by removing 'name' property.", + ], + }); + useEffect(() => { let newId; @@ -88,15 +105,16 @@ export function useField>( let field = form?.getFieldInstance(fieldName); - if (form) { - if (!field) { - field = form.createField(fieldName, true); - } + // if there is no field + if (form && !field && fieldName) { + field = form.createField(fieldName, true); } - if (field && field.rules !== rules) { - field.rules = rules; + if (field) { + // copy rules to the field rules + field.rules = [...(rules ?? [])]; + // if there are some rules and a delay then add a rule that delays the validation if (field.rules && field.rules.length && validationDelay) { field.rules.unshift(delayValidationRule(validationDelay)); } From a5a7e166d0c114c10d69565b1d35d30b6eb18fda Mon Sep 17 00:00:00 2001 From: Andrey Yamanov Date: Sat, 7 Dec 2024 12:16:12 +0100 Subject: [PATCH 03/11] fix(Form): internal logic fixes * 3 --- .../fields/Checkbox/checkbox-group.test.tsx | 4 ++-- .../fields/Checkbox/checkbox.test.tsx | 6 +++--- .../PasswordInput/password-input.test.tsx | 6 +++--- .../fields/RadioGroup/radio.test.tsx | 4 ++-- src/components/fields/Select/select.test.tsx | 12 +++++------ src/components/fields/Switch/switch.test.tsx | 6 +++--- .../fields/TextArea/textarea.test.tsx | 6 +++--- .../fields/TextInput/text-input.test.tsx | 6 +++--- .../form/Form/submit-error.test.tsx | 20 +++++++------------ src/components/form/Form/submit.test.tsx | 10 +++------- .../Bar/__tests__/notifications-bar.test.tsx | 6 ++---- .../NotificationView/notification.test.tsx | 10 ++++------ .../notification-list.test.tsx | 2 +- 13 files changed, 42 insertions(+), 56 deletions(-) diff --git a/src/components/fields/Checkbox/checkbox-group.test.tsx b/src/components/fields/Checkbox/checkbox-group.test.tsx index 8cbedd896..7d3e4f24d 100644 --- a/src/components/fields/Checkbox/checkbox-group.test.tsx +++ b/src/components/fields/Checkbox/checkbox-group.test.tsx @@ -30,7 +30,7 @@ describe('', () => { expect(checkbox[0]).toBeChecked(); expect(checkbox[1]).not.toBeChecked(); - await act(async () => await userEvent.click(checkbox[1])); + await userEvent.click(checkbox[1]); expect(checkbox[0]).toBeChecked(); expect(checkbox[1]).toBeChecked(); @@ -50,7 +50,7 @@ describe('', () => { ); const checkbox = getAllByRole('checkbox'); - await act(async () => await userEvent.click(checkbox[0])); + await userEvent.click(checkbox[0]); expect(checkbox[0]).toBeChecked(); expect(checkbox[1]).toBeChecked(); diff --git a/src/components/fields/Checkbox/checkbox.test.tsx b/src/components/fields/Checkbox/checkbox.test.tsx index 98b53bdf1..b4cb47670 100644 --- a/src/components/fields/Checkbox/checkbox.test.tsx +++ b/src/components/fields/Checkbox/checkbox.test.tsx @@ -12,7 +12,7 @@ describe('', () => { const { getByRole } = render(Test); const checkboxElement = getByRole('checkbox'); - await act(async () => await userEvent.click(checkboxElement)); + await userEvent.click(checkboxElement); expect(checkboxElement).toBeChecked(); }); @@ -24,7 +24,7 @@ describe('', () => { const checkboxElement = getByRole('checkbox'); - await act(async () => await userEvent.click(checkboxElement)); + await userEvent.click(checkboxElement); expect(checkboxElement).toBeChecked(); expect(formInstance.getFieldValue('test')).toBe(true); @@ -39,7 +39,7 @@ describe('', () => { const checkboxElement = getByRole('checkbox'); - await act(async () => await userEvent.click(checkboxElement)); + await userEvent.click(checkboxElement); expect(checkboxElement).toBeChecked(); expect(formInstance.getFieldValue('test')).toBe(true); diff --git a/src/components/fields/PasswordInput/password-input.test.tsx b/src/components/fields/PasswordInput/password-input.test.tsx index 1336621af..0a0fc4b8a 100644 --- a/src/components/fields/PasswordInput/password-input.test.tsx +++ b/src/components/fields/PasswordInput/password-input.test.tsx @@ -13,7 +13,7 @@ describe('', () => { const passwordInput = getByTestId('Input'); - await act(async () => await userEvent.type(passwordInput, 'test')); + await userEvent.type(passwordInput, 'test'); expect(passwordInput).toHaveValue('test'); }); @@ -25,7 +25,7 @@ describe('', () => { const passwordInput = getByTestId('Input'); - await act(async () => await userEvent.type(passwordInput, 'test')); + await userEvent.type(passwordInput, 'test'); expect(passwordInput).toHaveValue('test'); expect(formInstance.getFieldValue('test')).toBe('test'); @@ -40,7 +40,7 @@ describe('', () => { const passwordInput = getByTestId('Input'); - await act(async () => await userEvent.type(passwordInput, 'test')); + await userEvent.type(passwordInput, 'test'); expect(passwordInput).toHaveValue('test'); expect(formInstance.getFieldValue('test')).toBe('test'); diff --git a/src/components/fields/RadioGroup/radio.test.tsx b/src/components/fields/RadioGroup/radio.test.tsx index 4a1eb5a93..62f7ff313 100644 --- a/src/components/fields/RadioGroup/radio.test.tsx +++ b/src/components/fields/RadioGroup/radio.test.tsx @@ -19,7 +19,7 @@ describe(' and ', () => { , ); const radio = getAllByRole('radio'); - await act(async () => await userEvent.click(radio[0])); + await userEvent.click(radio[0]); expect(radio[0]).toBeChecked(); }); @@ -33,7 +33,7 @@ describe(' and ', () => { ); const radio = screen.getAllByRole('radio'); - await act(async () => await userEvent.click(radio[0])); + await userEvent.click(radio[0]); expect(radio[0]).toBeChecked(); diff --git a/src/components/fields/Select/select.test.tsx b/src/components/fields/Select/select.test.tsx index ff39f440f..b62b2a751 100644 --- a/src/components/fields/Select/select.test.tsx +++ b/src/components/fields/Select/select.test.tsx @@ -21,10 +21,10 @@ describe('', () => { ); const select = getByRole('button'); - await act(async () => await userEvent.click(select)); + await userEvent.click(select); const options = getAllByRole('option'); - await act(async () => await userEvent.click(options[1])); + await userEvent.click(options[1]); expect(select).toHaveTextContent('Red'); expect(formInstance.getFieldValue('test')).toBe('2'); @@ -60,10 +60,10 @@ describe('