From fc12948369108ce54fa1306e46bc25dc1286e67c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Wed, 15 Nov 2023 13:11:58 +0100 Subject: [PATCH 1/2] fix(FieldBlock): enhance fieldset/legend detection [Reprod](https://codesandbox.io/s/jolly-blackburn-32p58l?file=/src/App.tsx) Fixes #2893 --- .../getting-started/making-changes.mdx | 34 ++++++- .../forms/FieldBlock/FieldBlock.tsx | 61 +++++++++--- .../FieldBlock/__tests__/FieldBlock.test.tsx | 97 +++++++++++++++++++ 3 files changed, 175 insertions(+), 17 deletions(-) diff --git a/packages/dnb-design-system-portal/src/docs/contribute/getting-started/making-changes.mdx b/packages/dnb-design-system-portal/src/docs/contribute/getting-started/making-changes.mdx index 219fa0d2f13..50a4bcd86c3 100644 --- a/packages/dnb-design-system-portal/src/docs/contribute/getting-started/making-changes.mdx +++ b/packages/dnb-design-system-portal/src/docs/contribute/getting-started/making-changes.mdx @@ -148,9 +148,35 @@ function MyComponent(props: ComponentAllProps) { } ``` -### Form element support with `pickFormElementProps` +### "Form element" components -Form elements, like input, checkbox, slider etc. do support some form element properties. In order to support them, you can use `pickFormElementProps`, so only valid properties will effected the component. +Form elements, like input, checkbox, slider etc. should include some extra functionality in order to be used in various situations. + +Basically, components we would place inside a HTML `
` element. + +**Label vs fieldset/legend** + +They should be declared as a form element: + +```tsx +FormComponent._formElement = true +``` + +This helps e.g. to detect automated determination of label vs fieldset/legend. + +**Spacing** + +And they should be declared to support spacing props as well: + +```tsx +FormComponent._supportsSpacingProps = true +``` + +This is needed in order to fully support [Flex](/uilib/layout/flex/) layouts. + +#### Usage of `pickFormElementProps` + +In order to support form element props, such as `vertical` or `labelDirection`, you can use `pickFormElementProps`, so only valid properties will effected the component. ```tsx import { Context } from '../../shared' @@ -161,14 +187,14 @@ const defaultProps = { myParam: 'value', } -function MyComponent(props: Types) { +function FormComponent(props: Types) { const context = React.useContext(Context) const { myParam, skeleton, ...rest } = extendPropsWithContext( props, defaultProps, pickFormElementProps(context?.formElement) - context.MyComponent, + context.FormComponent, ) // Use myParam and spread the ...rest diff --git a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx index 3bb8537e78c..0299bc0b764 100644 --- a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx @@ -3,7 +3,10 @@ import classnames from 'classnames' import { Space, FormLabel, FormStatus } from '../../../components' import { FormError, ComponentProps, FieldProps } from '../types' import FieldBlockContext from './FieldBlockContext' -import { findElementInChildren } from '../../../shared/component-helper' +import { + findElementInChildren, + warn, +} from '../../../shared/component-helper' export type Props = Pick< FieldProps, @@ -128,17 +131,13 @@ function FieldBlock(props: Props) { ) // A child component with a label was found, use fieldset/legend instead of div/label - const enableFieldset = useMemo( - () => - label && - (asFieldset || - (!nestedFieldBlockContext && - findElementInChildren( - children, - (child: React.ReactElement) => child.props.label - ))), - [] - ) + const enableFieldset = useEnableFieldset({ + label, + forId, + asFieldset, + children, + nestedFieldBlockContext, + }) const state = error || warning || info const stateStatus = error @@ -153,7 +152,7 @@ function FieldBlock(props: Props) { return ( @@ -232,5 +231,41 @@ function FieldBlock(props: Props) { ) } +function useEnableFieldset({ + label, + forId, + asFieldset, + children, + nestedFieldBlockContext, +}) { + return useMemo(() => { + let result = asFieldset + + if (label && !result && !nestedFieldBlockContext) { + let count = 0 + + findElementInChildren(children, (child: React.ReactElement) => { + if ( + typeof child?.props?.label !== 'undefined' || + child?.type?.['_formElement'] === true + ) { + count++ + } + if (count > 1) { + return (result = true) + } + }) + + if (forId && count > 1) { + warn( + `You may not use forId="${forId}" as there where given several (${count}) form elements as children.` + ) + } + } + + return Boolean(result) + }, [children]) +} + FieldBlock._supportsSpacingProps = true export default FieldBlock diff --git a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/__tests__/FieldBlock.test.tsx b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/__tests__/FieldBlock.test.tsx index 0001ae80ff2..adba3be640c 100644 --- a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/__tests__/FieldBlock.test.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/__tests__/FieldBlock.test.tsx @@ -89,6 +89,30 @@ describe('FieldBlock', () => { ) }) + it('should warn when "forId" and several form elements where given', () => { + const orig = console.log + console.log = jest.fn() + + render( + + + + + ) + + expect(console.log).toHaveBeenCalledTimes(1) + expect(console.log).toHaveBeenCalledWith( + expect.anything(), + expect.stringContaining('forId="invalid"') + ) + + expect(document.querySelectorAll('fieldset')).toHaveLength(1) + expect(document.querySelectorAll('legend')).toHaveLength(1) + expect(document.querySelectorAll('label')).toHaveLength(2) + + console.log = orig + }) + it('should render a "label"', () => { render(content) @@ -182,6 +206,69 @@ describe('FieldBlock', () => { expect(labelElements[4]).toBe(undefined) }) + it('should use fieldset/legend elements when nested component has a label property', () => { + const { rerender } = render( + + + + + ) + + expect(document.querySelectorAll('fieldset')).toHaveLength(1) + expect(document.querySelectorAll('legend')).toHaveLength(1) + expect(document.querySelector('legend')).not.toHaveAttribute('for') + expect(document.querySelectorAll('label')).toHaveLength(2) + + rerender( + + + + + ) + + expect(document.querySelector('fieldset')).not.toBeInTheDocument() + expect(document.querySelector('legend')).not.toBeInTheDocument() + expect(document.querySelectorAll('label')).toHaveLength(2) + + rerender( + + + + ) + + expect(document.querySelector('fieldset')).not.toBeInTheDocument() + expect(document.querySelector('legend')).not.toBeInTheDocument() + expect(document.querySelectorAll('label')).toHaveLength(1) + }) + + it('should use fieldset/legend when _formElement is given', () => { + MockComponent._formElement = true + + const { rerender } = render( + + + + + ) + + expect(document.querySelectorAll('fieldset')).toHaveLength(1) + expect(document.querySelectorAll('legend')).toHaveLength(1) + expect(document.querySelector('legend')).not.toHaveAttribute('for') + + delete MockComponent._formElement + + rerender( + + + + + ) + + expect(document.querySelector('fieldset')).not.toBeInTheDocument() + expect(document.querySelector('legend')).not.toBeInTheDocument() + expect(document.querySelectorAll('label')).toHaveLength(1) + }) + it('should use fieldset/legend when "asFieldset" is given', () => { render( @@ -300,3 +387,13 @@ describe('FieldBlock', () => { expect(element.classList).toContain('custom-class') }) }) + +function MockComponent({ label = null, id = null }) { + return ( + <> + {label && } + + + ) +} +MockComponent._formElement = null From a44db468112ba82905afe1110813cd69f970d153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Wed, 15 Nov 2023 13:12:20 +0100 Subject: [PATCH 2/2] Add _formElement to form components --- .../dnb-eufemia/src/components/autocomplete/Autocomplete.js | 2 +- packages/dnb-eufemia/src/components/button/Button.js | 1 + packages/dnb-eufemia/src/components/checkbox/Checkbox.js | 1 + packages/dnb-eufemia/src/components/date-picker/DatePicker.js | 1 + packages/dnb-eufemia/src/components/dropdown/Dropdown.js | 2 +- packages/dnb-eufemia/src/components/form-label/FormLabel.tsx | 1 + .../dnb-eufemia/src/components/input-masked/InputMasked.js | 1 + .../dnb-eufemia/src/components/input-masked/MultiInputMask.tsx | 3 +++ packages/dnb-eufemia/src/components/input/Input.js | 1 + packages/dnb-eufemia/src/components/input/InputPassword.js | 1 + packages/dnb-eufemia/src/components/radio/Radio.js | 1 + packages/dnb-eufemia/src/components/radio/RadioGroup.js | 1 + packages/dnb-eufemia/src/components/slider/Slider.tsx | 1 + packages/dnb-eufemia/src/components/switch/Switch.js | 1 + packages/dnb-eufemia/src/components/tag/Tag.tsx | 1 + packages/dnb-eufemia/src/components/textarea/Textarea.js | 1 + .../dnb-eufemia/src/components/toggle-button/ToggleButton.js | 1 + .../src/components/toggle-button/ToggleButtonGroup.js | 1 + packages/dnb-eufemia/src/components/upload/Upload.tsx | 1 + 19 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/dnb-eufemia/src/components/autocomplete/Autocomplete.js b/packages/dnb-eufemia/src/components/autocomplete/Autocomplete.js index 1bed13d0be2..8cb0c19e5ca 100644 --- a/packages/dnb-eufemia/src/components/autocomplete/Autocomplete.js +++ b/packages/dnb-eufemia/src/components/autocomplete/Autocomplete.js @@ -2095,5 +2095,5 @@ class AutocompleteInstance extends React.PureComponent { } Autocomplete.HorizontalItem = DrawerList.HorizontalItem - +Autocomplete._formElement = true Autocomplete._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/button/Button.js b/packages/dnb-eufemia/src/components/button/Button.js index 28744f300c8..606fd2d19f5 100644 --- a/packages/dnb-eufemia/src/components/button/Button.js +++ b/packages/dnb-eufemia/src/components/button/Button.js @@ -470,4 +470,5 @@ Content.defaultProps = { isIconOnly: null, } +Button._formElement = true Button._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/checkbox/Checkbox.js b/packages/dnb-eufemia/src/components/checkbox/Checkbox.js index eeb06ef87ba..455aec15b12 100644 --- a/packages/dnb-eufemia/src/components/checkbox/Checkbox.js +++ b/packages/dnb-eufemia/src/components/checkbox/Checkbox.js @@ -372,4 +372,5 @@ CheckIcon.defaultProps = { size: 'default', } +CheckIcon._formElement = true CheckIcon._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/date-picker/DatePicker.js b/packages/dnb-eufemia/src/components/date-picker/DatePicker.js index b080519bc3a..013d267cfee 100644 --- a/packages/dnb-eufemia/src/components/date-picker/DatePicker.js +++ b/packages/dnb-eufemia/src/components/date-picker/DatePicker.js @@ -740,4 +740,5 @@ export default class DatePicker extends React.PureComponent { } } +DatePicker._formElement = true DatePicker._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/dropdown/Dropdown.js b/packages/dnb-eufemia/src/components/dropdown/Dropdown.js index 404f11ebb60..25c075a4d79 100644 --- a/packages/dnb-eufemia/src/components/dropdown/Dropdown.js +++ b/packages/dnb-eufemia/src/components/dropdown/Dropdown.js @@ -698,5 +698,5 @@ class DropdownInstance extends React.PureComponent { } Dropdown.HorizontalItem = DrawerList.HorizontalItem - +Dropdown._formElement = true Dropdown._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/form-label/FormLabel.tsx b/packages/dnb-eufemia/src/components/form-label/FormLabel.tsx index 693bbe8522b..04554d33f3a 100644 --- a/packages/dnb-eufemia/src/components/form-label/FormLabel.tsx +++ b/packages/dnb-eufemia/src/components/form-label/FormLabel.tsx @@ -112,4 +112,5 @@ export default function FormLabel(localProps: FormLabelAllProps) { return {text || children} } +FormLabel._formElement = true FormLabel._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/input-masked/InputMasked.js b/packages/dnb-eufemia/src/components/input-masked/InputMasked.js index bbc1c16f7d6..e8c47830be0 100644 --- a/packages/dnb-eufemia/src/components/input-masked/InputMasked.js +++ b/packages/dnb-eufemia/src/components/input-masked/InputMasked.js @@ -115,6 +115,7 @@ InputMasked.defaultProps = { on_submit_blur: null, } +InputMasked._formElement = true InputMasked._supportsSpacingProps = true export default InputMasked diff --git a/packages/dnb-eufemia/src/components/input-masked/MultiInputMask.tsx b/packages/dnb-eufemia/src/components/input-masked/MultiInputMask.tsx index 635c50009b0..a96aa920d6e 100644 --- a/packages/dnb-eufemia/src/components/input-masked/MultiInputMask.tsx +++ b/packages/dnb-eufemia/src/components/input-masked/MultiInputMask.tsx @@ -302,3 +302,6 @@ function MultiInputMaskInput({ } export default MultiInputMask + +MultiInputMask._formElement = true +MultiInputMask._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/input/Input.js b/packages/dnb-eufemia/src/components/input/Input.js index 2f25a3d8836..7a782b9a428 100644 --- a/packages/dnb-eufemia/src/components/input/Input.js +++ b/packages/dnb-eufemia/src/components/input/Input.js @@ -777,4 +777,5 @@ InputIcon.propTypes = { ]).isRequired, } +Input._formElement = true Input._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/input/InputPassword.js b/packages/dnb-eufemia/src/components/input/InputPassword.js index a23ae223b1e..b906ecf9f19 100644 --- a/packages/dnb-eufemia/src/components/input/InputPassword.js +++ b/packages/dnb-eufemia/src/components/input/InputPassword.js @@ -128,4 +128,5 @@ export default class InputPassword extends React.PureComponent { } } +InputPassword._formElement = true InputPassword._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/radio/Radio.js b/packages/dnb-eufemia/src/components/radio/Radio.js index df7269cdad7..5b49698bbfa 100644 --- a/packages/dnb-eufemia/src/components/radio/Radio.js +++ b/packages/dnb-eufemia/src/components/radio/Radio.js @@ -461,4 +461,5 @@ export default class Radio extends React.PureComponent { } } +Radio._formElement = true Radio._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/radio/RadioGroup.js b/packages/dnb-eufemia/src/components/radio/RadioGroup.js index 283a2a91963..cb820366c79 100644 --- a/packages/dnb-eufemia/src/components/radio/RadioGroup.js +++ b/packages/dnb-eufemia/src/components/radio/RadioGroup.js @@ -294,4 +294,5 @@ export default class RadioGroup extends React.PureComponent { } } +RadioGroup._formElement = true RadioGroup._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/slider/Slider.tsx b/packages/dnb-eufemia/src/components/slider/Slider.tsx index 7b919f3b90d..85f15d828c1 100644 --- a/packages/dnb-eufemia/src/components/slider/Slider.tsx +++ b/packages/dnb-eufemia/src/components/slider/Slider.tsx @@ -20,6 +20,7 @@ function Slider(localProps: SliderAllProps) { ) } +Slider._formElement = true Slider._supportsSpacingProps = true export default Slider diff --git a/packages/dnb-eufemia/src/components/switch/Switch.js b/packages/dnb-eufemia/src/components/switch/Switch.js index 48a252ba5b0..b4d61dc9b68 100644 --- a/packages/dnb-eufemia/src/components/switch/Switch.js +++ b/packages/dnb-eufemia/src/components/switch/Switch.js @@ -363,4 +363,5 @@ export default class Switch extends React.PureComponent { } } +Switch._formElement = true Switch._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/tag/Tag.tsx b/packages/dnb-eufemia/src/components/tag/Tag.tsx index 4c63b7867fb..fed0cf19747 100644 --- a/packages/dnb-eufemia/src/components/tag/Tag.tsx +++ b/packages/dnb-eufemia/src/components/tag/Tag.tsx @@ -206,6 +206,7 @@ const Tag = (localProps: TagProps & SpacingProps) => { Tag.Group = TagGroup +Tag._formElement = true Tag._supportsSpacingProps = true export default Tag diff --git a/packages/dnb-eufemia/src/components/textarea/Textarea.js b/packages/dnb-eufemia/src/components/textarea/Textarea.js index efd8ab5f41b..7c6f29f4d07 100644 --- a/packages/dnb-eufemia/src/components/textarea/Textarea.js +++ b/packages/dnb-eufemia/src/components/textarea/Textarea.js @@ -543,4 +543,5 @@ export default class Textarea extends React.PureComponent { } } +Textarea._formElement = true Textarea._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/toggle-button/ToggleButton.js b/packages/dnb-eufemia/src/components/toggle-button/ToggleButton.js index 935ad25ab0b..11f54c3fc23 100644 --- a/packages/dnb-eufemia/src/components/toggle-button/ToggleButton.js +++ b/packages/dnb-eufemia/src/components/toggle-button/ToggleButton.js @@ -501,4 +501,5 @@ export default class ToggleButton extends React.PureComponent { } } +ToggleButton._formElement = true ToggleButton._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/toggle-button/ToggleButtonGroup.js b/packages/dnb-eufemia/src/components/toggle-button/ToggleButtonGroup.js index 5c96998a12b..c4edc78734c 100644 --- a/packages/dnb-eufemia/src/components/toggle-button/ToggleButtonGroup.js +++ b/packages/dnb-eufemia/src/components/toggle-button/ToggleButtonGroup.js @@ -350,4 +350,5 @@ export default class ToggleButtonGroup extends React.PureComponent { } } +ToggleButtonGroup._formElement = true ToggleButtonGroup._supportsSpacingProps = true diff --git a/packages/dnb-eufemia/src/components/upload/Upload.tsx b/packages/dnb-eufemia/src/components/upload/Upload.tsx index 95be43afd58..3c5b9fea5a4 100644 --- a/packages/dnb-eufemia/src/components/upload/Upload.tsx +++ b/packages/dnb-eufemia/src/components/upload/Upload.tsx @@ -141,6 +141,7 @@ const Upload = (localProps: UploadAllProps) => { Upload.useUpload = useUpload +Upload._formElement = true Upload._supportsSpacingProps = true export default Upload