From 66736f59ba74a82088d14eae0e95ff2f0effc3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Wed, 25 Sep 2024 07:24:02 +0200 Subject: [PATCH] Fix propper Wizard and defaultValue support --- .../extensions/forms/Iterate/Array/Array.tsx | 17 +- .../Iterate/Array/__tests__/Array.test.tsx | 133 ++++++++++++---- .../Iterate/PushContainer/PushContainer.tsx | 27 +++- .../PushContainer/PushContainerDocs.ts | 10 ++ .../__tests__/PushContainer.test.tsx | 2 +- .../forms/Iterate/stories/Iterate.stories.tsx | 41 ++++- .../src/extensions/forms/Tools/Log.tsx | 2 +- .../__tests__/WizardContainer.test.tsx | 105 ++++++++++++- .../extensions/forms/hooks/useFieldProps.ts | 148 ++++++++++++------ .../src/extensions/forms/hooks/usePath.ts | 40 +++-- 10 files changed, 407 insertions(+), 118 deletions(-) diff --git a/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/Array.tsx b/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/Array.tsx index c906ce14d50..c7b72e0faa4 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/Array.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/Array.tsx @@ -24,7 +24,6 @@ import IterateItemContext, { import SummaryListContext from '../../Value/SummaryList/SummaryListContext' import ValueBlockContext from '../../ValueBlock/ValueBlockContext' import FieldBoundaryProvider from '../../DataContext/FieldBoundary/FieldBoundaryProvider' -import DataContext from '../../DataContext/Context' import useDataValue from '../../hooks/useDataValue' import { useArrayLimit, useSwitchContainerMode } from '../hooks' import { getMessage } from '../../FieldBlock' @@ -90,7 +89,6 @@ function ArrayComponent(props: Props) { value: arrayValue, limit, error, - defaultValue, withoutFlex, emptyValue, placeholder, @@ -100,7 +98,11 @@ function ArrayComponent(props: Props) { setChanged, onChange, children, - } = useFieldProps(preparedProps) + } = useFieldProps(preparedProps, { + // To ensure the defaultValue set on the Iterate.Array is set in the data context, + // and will not overwrite defaultValues set by fields inside the Iterate.Array. + updateContextDataInSync: true, + }) useMountEffect(() => { // To ensure the validator is called when a new item is added @@ -129,15 +131,6 @@ function ArrayComponent(props: Props) { const omitFlex = withoutFlex ?? (summaryListContext || valueBlockContext) - // To support React.StrictMode, we inject the defaultValue into the data context this way. - // The routine inside useFieldProps where updateDataValueDataContext is called, does not support React.StrictMode - const { handlePathChange } = useContext(DataContext) || {} - useMountEffect(() => { - if (defaultValue) { - handlePathChange?.(path, defaultValue) - } - }) - useEffect(() => { // Update inside the useEffect, to support React.StrictMode valueCountRef.current = arrayValue || [] diff --git a/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/__tests__/Array.test.tsx b/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/__tests__/Array.test.tsx index 73d8afc2f66..63e8db3b773 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/__tests__/Array.test.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Iterate/Array/__tests__/Array.test.tsx @@ -562,31 +562,110 @@ describe('Iterate.Array', () => { expect(onChangeIterate).toHaveBeenLastCalledWith(['foo']) }) - it('should handle "defaultValue" with React.StrictMode', () => { - const onSubmit = jest.fn() + describe('defaultValue', () => { + it('should handle "defaultValue" (empty string) in React.StrictMode', () => { + const onSubmit = jest.fn() - render( - - - - - - - - ) + render( + + + + + + + + ) - const form = document.querySelector('form') - const input = document.querySelector('input') + const form = document.querySelector('form') + const input = document.querySelector('input') - expect(input).toHaveValue('') + expect(input).toHaveValue('') - fireEvent.submit(form) + fireEvent.submit(form) - expect(onSubmit).toHaveBeenCalledTimes(1) - expect(onSubmit).toHaveBeenLastCalledWith( - { myList: [''] }, - expect.anything() - ) + expect(onSubmit).toHaveBeenCalledTimes(1) + expect(onSubmit).toHaveBeenLastCalledWith( + { myList: [''] }, + expect.anything() + ) + }) + + it('should handle "defaultValue" (with value) in React.StrictMode', () => { + const onSubmit = jest.fn() + + render( + + + + + + + + ) + + const form = document.querySelector('form') + const input = document.querySelector('input') + + expect(input).toHaveValue('foo') + + fireEvent.submit(form) + + expect(onSubmit).toHaveBeenCalledTimes(1) + expect(onSubmit).toHaveBeenLastCalledWith( + { myList: ['foo'] }, + expect.anything() + ) + }) + + it('should handle "defaultValue" (with null) in React.StrictMode', () => { + const onSubmit = jest.fn() + + render( + + + + + + + + ) + + const form = document.querySelector('form') + const input = document.querySelector('input') + + expect(input).toHaveValue('foo') + + fireEvent.submit(form) + + expect(onSubmit).toHaveBeenCalledTimes(1) + expect(onSubmit).toHaveBeenLastCalledWith( + { myList: ['foo'] }, + expect.anything() + ) + }) + + it('should set empty array in the data context', () => { + const onSubmit = jest.fn() + + render( + + + + content + + + + ) + + const form = document.querySelector('form') + fireEvent.submit(form) + + expect(onSubmit).toHaveBeenCalledTimes(1) + expect(onSubmit).toHaveBeenLastCalledWith( + { myList: [] }, + expect.anything() + ) + }) }) describe('with primitive elements', () => { @@ -1294,7 +1373,7 @@ describe('Iterate.Array', () => { @@ -1311,26 +1390,20 @@ describe('Iterate.Array', () => { ) const form = document.querySelector('form') - const [first, second, third, forth] = Array.from( + const [first, second, third] = Array.from( document.querySelectorAll('input') ) expect(first).toHaveValue('default value 1') expect(second).toHaveValue('default value 2') - expect(third).toHaveValue('default value 3') - expect(forth).toHaveValue('default value 4') + expect(third).toHaveValue('something') fireEvent.submit(form) expect(onSubmit).toHaveBeenCalledTimes(1) expect(onSubmit).toHaveBeenLastCalledWith( { - myList: [ - 'default value 1', - 'default value 2', - 'default value 3', - 'default value 4', - ], + myList: ['default value 1', 'default value 2', 'something'], }, expect.anything() ) diff --git a/packages/dnb-eufemia/src/extensions/forms/Iterate/PushContainer/PushContainer.tsx b/packages/dnb-eufemia/src/extensions/forms/Iterate/PushContainer/PushContainer.tsx index 0db2e71b022..4cfff66df79 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Iterate/PushContainer/PushContainer.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Iterate/PushContainer/PushContainer.tsx @@ -3,7 +3,6 @@ import Isolation from '../../Form/Isolation' import PushContainerContext from './PushContainerContext' import IterateItemContext from '../IterateItemContext' import DataContext from '../../DataContext/Context' -import { clearedData } from '../../DataContext/Provider' import useDataValue from '../../hooks/useDataValue' import EditContainer, { CancelButton, DoneButton } from '../EditContainer' import IterateArray, { ContainerMode } from '../Array' @@ -43,6 +42,11 @@ export type Props = { */ data?: unknown | Record + /** + * Prefilled data to add to the fields. + */ + defaultData?: unknown | Record + /** * A custom toolbar to be shown below the container. */ @@ -58,7 +62,8 @@ export type AllProps = Props & SpacingProps & ArrayItemAreaProps function PushContainer(props: AllProps) { const { - data, + data: dataProp, + defaultData: defaultDataProp, path, title, children, @@ -92,14 +97,24 @@ function PushContainer(props: AllProps) { } const { getValueByPath } = useDataValue() + const getFallback = useCallback(() => { + return Array.isArray(getValueByPath(path)) ? null : {} + }, [getValueByPath, path]) + + const data = useMemo(() => { + if (defaultDataProp) { + return // don't return a fallback, because we want to use the defaultData + } + return { newItems: [dataProp ?? getFallback()] } + }, [dataProp, defaultDataProp, getFallback]) + const defaultData = useMemo(() => { - const value = - data ?? (Array.isArray(getValueByPath(path)) ? null : clearedData) - return { newItems: [value] } - }, [data, getValueByPath, path]) + return { newItems: [defaultDataProp ?? getFallback()] } + }, [defaultDataProp, getFallback]) return ( { render( - + diff --git a/packages/dnb-eufemia/src/extensions/forms/Iterate/stories/Iterate.stories.tsx b/packages/dnb-eufemia/src/extensions/forms/Iterate/stories/Iterate.stories.tsx index 141db4d1067..85179810776 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Iterate/stories/Iterate.stories.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Iterate/stories/Iterate.stories.tsx @@ -1,5 +1,5 @@ import React, { useCallback } from 'react' -import { Field, Form, Iterate, Value } from '../..' +import { Field, Form, Iterate, Tools, Value, Wizard } from '../..' import { Card, Flex, Section } from '../../../../components' export default { @@ -256,3 +256,42 @@ export const WithArrayValidator = () => { ) } + +export function InWizard() { + return ( + + + + + + + + + + + + + + + + + + + + + + + + + + + ) +} diff --git a/packages/dnb-eufemia/src/extensions/forms/Tools/Log.tsx b/packages/dnb-eufemia/src/extensions/forms/Tools/Log.tsx index dfe5302a714..c7bf9d7db93 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Tools/Log.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Tools/Log.tsx @@ -1,4 +1,4 @@ -import { useContext } from 'react' +import React, { useContext } from 'react' import DataContext from '../DataContext/Context' import Section, { SectionProps } from '../../../components/Section' diff --git a/packages/dnb-eufemia/src/extensions/forms/Wizard/Container/__tests__/WizardContainer.test.tsx b/packages/dnb-eufemia/src/extensions/forms/Wizard/Container/__tests__/WizardContainer.test.tsx index 5eefdaf0be5..1cc6df7b644 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Wizard/Container/__tests__/WizardContainer.test.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Wizard/Container/__tests__/WizardContainer.test.tsx @@ -2,7 +2,7 @@ import React from 'react' import { fireEvent, render, screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { wait } from '../../../../../core/jest/jestSetup' -import { Field, Form, OnSubmit, Wizard } from '../../..' +import { Field, Form, Iterate, OnSubmit, Wizard } from '../../..' import nbNO from '../../../constants/locales/nb-NO' const nb = nbNO['nb-NO'] @@ -1964,4 +1964,107 @@ describe('Wizard.Container', () => { expect(iframe.parentElement).toBeNull() }) }) + + describe('defaultValue', () => { + it('should set defaultValue of a Field.* only once between step changes', async () => { + const onChange = jest.fn() + const onStepChange = jest.fn() + + render( + + + + + + + + + + + + + ) + + expect(document.querySelector('input')).toHaveValue('123') + + await userEvent.type(document.querySelector('input'), '4') + + expect(document.querySelector('input')).toHaveValue('1234') + + await userEvent.click(nextButton()) + + expect(onStepChange).toHaveBeenLastCalledWith( + 1, + 'next', + expect.anything() + ) + + await userEvent.click(previousButton()) + + expect(onStepChange).toHaveBeenLastCalledWith( + 0, + 'previous', + expect.anything() + ) + + expect(document.querySelector('input')).toHaveValue('1234') + }) + + it('should set defaultValue of Iterate.Array only once between step changes', async () => { + const onChange = jest.fn() + const onStepChange = jest.fn() + + render( + + + + + + + + + + + + + + + + + + ) + + expect(document.querySelectorAll('input')).toHaveLength(1) + expect(document.querySelector('input')).toHaveValue('123') + + await userEvent.type(document.querySelector('input'), '4') + + expect(document.querySelector('input')).toHaveValue('1234') + + const pushButton = document.querySelector( + '.dnb-forms-iterate-push-button' + ) + await userEvent.click(pushButton) + + expect(document.querySelectorAll('input')).toHaveLength(2) + + await userEvent.click(nextButton()) + + expect(onStepChange).toHaveBeenLastCalledWith( + 1, + 'next', + expect.anything() + ) + + await userEvent.click(previousButton()) + + expect(document.querySelectorAll('input')).toHaveLength(2) + expect(onStepChange).toHaveBeenLastCalledWith( + 0, + 'previous', + expect.anything() + ) + expect(document.querySelector('input')).toHaveValue('1234') + }) + }) }) diff --git a/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts b/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts index 436c16cefcb..273a2013c66 100644 --- a/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts +++ b/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts @@ -82,7 +82,10 @@ export type DataAttributes = { export default function useFieldProps( localeProps: Props & FieldPropsGeneric, - { executeOnChangeRegardlessOfError = false } = {} + { + executeOnChangeRegardlessOfError = false, + updateContextDataInSync = false, + } = {} ): typeof localeProps & ReturnAdditional { const { extend } = useContext(FieldPropsContext) const props = extend(localeProps) @@ -1565,19 +1568,23 @@ export default function useFieldProps( // Use "useLayoutEffect" to avoid flickering when value/defaultValue gets set, and other fields dependent on it. // Form.Visibility is an example of a logic, where a field value/defaultValue can be used to set the set state of a path, // where again other fields depend on it. - const tmpValueRef = useRef>({}) - const setData = useCallback(() => { + const tmpTransValueRef = useRef>({}) + const setContextData = useCallback(() => { if (hasPath || hasItemPath) { - let value = valueProp + let valueToStore = valueProp + + const data = + hasItemPath || updateContextDataInSync + ? dataContext.internalDataRef?.current + : dataContext.data // First, look for existing data in the context - const hasValue = - pointer.has(dataContext.data, identifier) || identifier === '/' + const hasValue = pointer.has(data, identifier) || identifier === '/' const existingValue = identifier === '/' - ? dataContext.data + ? data : hasValue - ? pointer.get(dataContext.data, identifier) + ? pointer.get(data, identifier) : undefined // If no data where found in the dataContext, look for shared data @@ -1585,34 +1592,30 @@ export default function useFieldProps( dataContext.id && !hasValue && typeof existingValue === 'undefined' && - typeof value === 'undefined' + typeof valueToStore === 'undefined' ) { const sharedState = createSharedState(dataContext.id) const hasValue = pointer.has(sharedState.data, identifier) if (hasValue) { const sharedValue = pointer.get(sharedState.data, identifier) if (sharedValue) { - value = sharedValue + valueToStore = sharedValue } } } const hasDefaultValue = typeof defaultValueRef.current !== 'undefined' && - typeof value === 'undefined' + typeof valueToStore === 'undefined' if (hasDefaultValue) { - value = defaultValueRef.current + valueToStore = defaultValueRef.current defaultValueRef.current = undefined } if (hasItemPath) { - if (hasDefaultValue) { - // When an itemPath is given, we rather want to use the defaultValue, - // even if the data context exists. Because the defaultValue should now be set in the data context. - valueRef.current = value - } else if ( - typeof value === 'undefined' && + if ( + typeof valueToStore === 'undefined' && typeof existingValue !== 'undefined' ) { // On the rerender (after defaultValue was set) and the data context was given, but as "undefined", @@ -1620,53 +1623,86 @@ export default function useFieldProps( // because else the comparison "valueRef.current !== existingValue" is true and we will set undefined as the new data context value. valueRef.current = existingValue } + + if (hasDefaultValue && Array.isArray(existingValue)) { + // To ensure we actually update the data context value later, + // and not stop in the "if" statement below (valueRef.current === existingValue). + // Also, valueRef.current may actually be the array value too. + valueRef.current = valueToStore + } } + if (updateContextDataInSync) { + // When an array is given (iterate), we don't want to overwrite the existing array + if (hasDefaultValue && hasValue) { + return // stop here, we don't want to overwrite the existing array + } + + // React.StrictMode will come with "undefined" on the second render, + // because "defaultValueRef.current" was removed. + // But because we run "useMemo" on the first render when updateContextDataInSync is true, + // we have still a valid value/array. + if (!Array.isArray(valueToStore)) { + return // stop here, never use a non-array value when in "updateContextDataInSync" + } + } + + // if (!hasValue && typeof valueToStore === 'undefined') { + // return // stop here, we don't want to set "undefined" as the value + // } + if ( - (!hasValue && !hasItemPath) || - (value !== existingValue && + hasValue && + (valueToStore === existingValue || // Prevents an infinite loop by skipping the update if the value hasn't changed - valueRef.current !== existingValue) + valueRef.current === existingValue) ) { - if ( - identifier in tmpValueRef.current && - tmpValueRef.current[identifier] === value - ) { - return // stop here, avoid infinite loop - } + return // stop here, we don't want to set same value twice + } - const transformedValue = transformers.current.transformOut( - value, - transformers.current.provideAdditionalArgs(value) - ) - if (transformedValue !== value) { - // When the value got transformed, we want to update the internal value, and avoid an infinite loop - tmpValueRef.current[identifier] = value - value = transformedValue - } + if ( + identifier in tmpTransValueRef.current && + tmpTransValueRef.current[identifier] === valueToStore + ) { + return // stop here, avoid infinite loop + } - // When an itemPath is given, we don't want to rerender the context on every iteration because of performance reasons. - // We know when the last item is reached, so we can prevent rerenders during the iteration. - const preventUpdate = - hasItemPath && iterateIndex < iterateArrayValue?.length - 1 + const transformedValue = transformers.current.transformOut( + valueToStore, + transformers.current.provideAdditionalArgs(valueToStore) + ) + if (transformedValue !== valueToStore) { + // When the value got transformed, we want to update the internal value, and avoid an infinite loop + tmpTransValueRef.current[identifier] = valueToStore + valueToStore = transformedValue + } - // Update the data context when a pointer not exists, - // but was given initially. - updateDataValueDataContext?.(identifier, value, { preventUpdate }) + // When an itemPath is given, we don't want to rerender the context on every iteration because of performance reasons. + // We know when the last item is reached, so we can prevent rerenders during the iteration. + const preventUpdate = + updateContextDataInSync || + (hasItemPath && iterateIndex < iterateArrayValue?.length - 1) - if (!preventUpdate) { - validateDataDataContext?.() - } + // Update the data context when a pointer not exists, + // but was given initially. + updateDataValueDataContext?.(identifier, valueToStore, { + preventUpdate, + }) + + if (!preventUpdate) { + validateDataDataContext?.() } } }, [ dataContext.data, dataContext.id, + dataContext.internalDataRef, hasItemPath, hasPath, identifier, iterateArrayValue?.length, iterateIndex, + updateContextDataInSync, updateDataValueDataContext, validateDataDataContext, valueProp, @@ -1698,18 +1734,26 @@ export default function useFieldProps( } }, [clearErrorState, defaultValue, hideError, isEmptyData]) + useMemo(() => { + if (updateContextDataInSync && !isEmptyData()) { + setContextData() + } + }, [isEmptyData, updateContextDataInSync, setContextData]) + useLayoutEffect(() => { - if (!isEmptyData()) { - setData() + if (!updateContextDataInSync && !isEmptyData()) { + setContextData() } - }, [isEmptyData, setData]) + }, [isEmptyData, updateContextDataInSync, setContextData]) useEffect(() => { if (isEmptyData()) { - setData() - validateValue() + requestAnimationFrame(() => { + setContextData() + validateValue() + }) } - }, [isEmptyData, setData, validateValue]) + }, [isEmptyData, setContextData, validateValue]) useEffect(() => { if (showAllErrors || showBoundaryErrors) { diff --git a/packages/dnb-eufemia/src/extensions/forms/hooks/usePath.ts b/packages/dnb-eufemia/src/extensions/forms/hooks/usePath.ts index 24f1406272b..e97edbf57b2 100644 --- a/packages/dnb-eufemia/src/extensions/forms/hooks/usePath.ts +++ b/packages/dnb-eufemia/src/extensions/forms/hooks/usePath.ts @@ -24,20 +24,29 @@ export default function usePath(props: Props = {}) { throw new Error(`itemPath="${itemPathProp}" must start with a slash`) } - const joinPath = useCallback((paths: Array) => { - return paths - .reduce((acc, cur) => (cur ? `${acc}/${cur}` : acc), '/') - .replace(/\/{2,}/g, '/') - .replace(/\/+$/, '') - }, []) + // Will remove duplicate slashes and trailing slashes + // /foo///bar/// => /foo/bar + const cleanPath = useCallback( + (path: Path) => path.replace(/\/+$|\/(\/)+/g, '$1'), + [] + ) + + const joinPath = useCallback( + (paths: Array) => { + return cleanPath( + paths.reduce((acc, cur) => (cur ? `${acc}/${cur}` : acc), '/') + ) + }, + [cleanPath] + ) const makeSectionPath = useCallback( (path: Path) => { - return `${ - sectionPath && sectionPath !== '/' ? sectionPath : '' - }${path}`.replace(/\/$/, '') + return cleanPath( + `${sectionPath && sectionPath !== '/' ? sectionPath : ''}${path}` + ) }, - [sectionPath] + [cleanPath, sectionPath] ) const makeIteratePath = useCallback( @@ -51,15 +60,18 @@ export default function usePath(props: Props = {}) { root = makeSectionPath('') } - return `${root}${iteratePath || ''}/${iterateElementIndex}${ - itemPath && itemPath !== '/' ? itemPath : '' - }` + return cleanPath( + `${root}${iteratePath || ''}/${iterateElementIndex}${ + itemPath || '' + }` + ) }, [ + itemPathProp, iteratePathProp, sectionPath, + cleanPath, iterateElementIndex, - itemPathProp, makeSectionPath, ] )