From 4eefca5fb5f89f4d0c35f0e46e64bf08586e11fb Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Tue, 27 Jun 2023 23:00:32 +0100 Subject: [PATCH 1/4] Adding support for defined IDs in `TextControl` This patch adds support for custom IDs in the `TextControl` component. Currently any passed ID prop is ignored, and a auto-generated value is used instead. --- Resolves #24749 --- .../components/src/text-control/index.tsx | 11 ++++- .../src/text-control/test/text-control.tsx | 46 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 packages/components/src/text-control/test/text-control.tsx diff --git a/packages/components/src/text-control/index.tsx b/packages/components/src/text-control/index.tsx index 15d792489ba99e..e2f85eef07f562 100644 --- a/packages/components/src/text-control/index.tsx +++ b/packages/components/src/text-control/index.tsx @@ -16,6 +16,13 @@ import BaseControl from '../base-control'; import type { WordPressComponentProps } from '../ui/context'; import type { TextControlProps } from './types'; +function useUniqueId( idProp?: string ) { + const instanceId = useInstanceId( TextControl ); + const id = `inspector-text-control-${ instanceId }`; + + return idProp || id; +} + function UnforwardedTextControl( props: WordPressComponentProps< TextControlProps, 'input', false >, ref: ForwardedRef< HTMLInputElement > @@ -26,13 +33,13 @@ function UnforwardedTextControl( hideLabelFromVision, value, help, + id: idProp, className, onChange, type = 'text', ...additionalProps } = props; - const instanceId = useInstanceId( TextControl ); - const id = `inspector-text-control-${ instanceId }`; + const id = useUniqueId( idProp ); const onChangeValue = ( event: ChangeEvent< HTMLInputElement > ) => onChange( event.target.value ); diff --git a/packages/components/src/text-control/test/text-control.tsx b/packages/components/src/text-control/test/text-control.tsx new file mode 100644 index 00000000000000..d252e1796953b4 --- /dev/null +++ b/packages/components/src/text-control/test/text-control.tsx @@ -0,0 +1,46 @@ +/** + * External dependencies + */ +import { render, screen } from '@testing-library/react'; + +/** + * Internal dependencies + */ +import TextControl from '..'; + +const noop = () => {}; + +describe( 'TextControl', () => { + it( 'should generate an ID if not passed as a prop', () => { + render( ); + + expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( + 'id', + expect.stringMatching( /^inspector-text-control-/ ) + ); + } ); + + it( 'should use the passed ID prop if provided', () => { + const id = 'test-id'; + render( ); + + expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( 'id', id ); + } ); + + it( 'should map the label and input together when given an ID', () => { + const id = 'test-id'; + const labelValue = 'Test Label'; + render( + + ); + + const textbox = screen.getByRole( 'textbox' ); + const label = screen.getByText( labelValue ); + expect( textbox ).toHaveAttribute( 'id', label.getAttribute( 'for' ) ); + } ); +} ); From 47f6089a92510d209872a3d082423809b5321ce6 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Tue, 11 Jul 2023 16:49:22 +0200 Subject: [PATCH 2/4] Updating tests Changing the test setup for labels to be more `testing-library` idiomatic. --- .../src/text-control/test/text-control.tsx | 69 +++++++++++-------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/packages/components/src/text-control/test/text-control.tsx b/packages/components/src/text-control/test/text-control.tsx index d252e1796953b4..fc048b93992f08 100644 --- a/packages/components/src/text-control/test/text-control.tsx +++ b/packages/components/src/text-control/test/text-control.tsx @@ -11,36 +11,51 @@ import TextControl from '..'; const noop = () => {}; describe( 'TextControl', () => { - it( 'should generate an ID if not passed as a prop', () => { - render( ); - - expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( - 'id', - expect.stringMatching( /^inspector-text-control-/ ) - ); + describe( 'When no ID prop is provided', () => { + it( 'should generate an ID', () => { + render( ); + + expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( + 'id', + expect.stringMatching( /^inspector-text-control-/ ) + ); + } ); + + it( 'should be labelled correctly', () => { + const labelValue = 'Test Label'; + render( + + ); + + expect( + screen.getByRole( 'textbox', { name: labelValue } ) + ).toBeVisible(); + } ); } ); - it( 'should use the passed ID prop if provided', () => { + describe( 'When an ID prop is provided', () => { const id = 'test-id'; - render( ); - - expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( 'id', id ); - } ); - it( 'should map the label and input together when given an ID', () => { - const id = 'test-id'; - const labelValue = 'Test Label'; - render( - - ); - - const textbox = screen.getByRole( 'textbox' ); - const label = screen.getByText( labelValue ); - expect( textbox ).toHaveAttribute( 'id', label.getAttribute( 'for' ) ); + it( 'should use the passed ID prop if provided', () => { + render( ); + + expect( screen.getByRole( 'textbox' ) ).toHaveAttribute( 'id', id ); + } ); + + it( 'should be labelled correctly', () => { + const labelValue = 'Test Label'; + render( + + ); + + expect( + screen.getByRole( 'textbox', { name: labelValue } ) + ).toBeVisible(); + } ); } ); } ); From 7f14443aedc6bdd2d135413cc677ffc806026d8d Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Tue, 11 Jul 2023 16:55:33 +0200 Subject: [PATCH 3/4] Updating CHANGELOG Adding line item under Enhancements to include `TextControl` changes. --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 6409ce7515646f..e8b92bfd89b078 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -10,6 +10,7 @@ - `Button`: Introduce `size` prop with `default`, `compact`, and `small` variants ([#51842](https://github.com/WordPress/gutenberg/pull/51842)). - `ItemGroup`: Update button focus state styles to be inline with other button focus states in the editor. ([#51576](https://github.com/WordPress/gutenberg/pull/51576)). - `ItemGroup`: Update button focus state styles to target `:focus-visible` rather than `:focus`. ([#51787](https://github.com/WordPress/gutenberg/pull/51787)). +- `TextControl`: Add `id` prop to allow for custom IDs in `TextControl`s ([#52028](https://github.com/WordPress/gutenberg/pull/52028)). ### Bug Fix From 9da368df18f970c2945fe4bd9ae29f758f5f368d Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 12 Jul 2023 16:02:10 +0200 Subject: [PATCH 4/4] Fixing use of `useInstanceId` Dropping the old `useUniqueId` pattern in favour of the more succinct and complete `useInstanceId` usage. --- packages/components/src/text-control/index.tsx | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/components/src/text-control/index.tsx b/packages/components/src/text-control/index.tsx index e2f85eef07f562..34c9028c1cb8bc 100644 --- a/packages/components/src/text-control/index.tsx +++ b/packages/components/src/text-control/index.tsx @@ -16,13 +16,6 @@ import BaseControl from '../base-control'; import type { WordPressComponentProps } from '../ui/context'; import type { TextControlProps } from './types'; -function useUniqueId( idProp?: string ) { - const instanceId = useInstanceId( TextControl ); - const id = `inspector-text-control-${ instanceId }`; - - return idProp || id; -} - function UnforwardedTextControl( props: WordPressComponentProps< TextControlProps, 'input', false >, ref: ForwardedRef< HTMLInputElement > @@ -39,7 +32,7 @@ function UnforwardedTextControl( type = 'text', ...additionalProps } = props; - const id = useUniqueId( idProp ); + const id = useInstanceId( TextControl, 'inspector-text-control', idProp ); const onChangeValue = ( event: ChangeEvent< HTMLInputElement > ) => onChange( event.target.value );