Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RadioControl: add ref forwarding #41641

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
- `TreeSelect`: Convert to TypeScript ([#41536](https://github.com/WordPress/gutenberg/pull/41536)).
- `FontSizePicker`: updated to satisfy `react/exhuastive-deps` eslint rule ([#41600](https://github.com/WordPress/gutenberg/pull/41600)).

### Enhancements

- `RadioControl`: add ref forwarding ([#41641](https://github.com/WordPress/gutenberg/pull/41641)).

## 19.12.0 (2022-06-01)

Expand Down
66 changes: 33 additions & 33 deletions packages/components/src/radio-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
*/
import { isEmpty } from 'lodash';
import classnames from 'classnames';
import type { ChangeEvent } from 'react';
import type { ChangeEvent, ForwardedRef } from 'react';

/**
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { forwardRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -17,38 +18,9 @@ import BaseControl from '../base-control';
import type { WordPressComponentProps } from '../ui/context';
import type { RadioControlProps } from './types';

/**
* Render a user interface to select the user type using radio inputs.
*
* ```jsx
* import { RadioControl } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* const MyRadioControl = () => {
* const [ option, setOption ] = useState( 'a' );
*
* return (
* <RadioControl
* label="User type"
* help="The type of the current user"
* selected={ option }
* options={ [
* { label: 'Author', value: 'a' },
* { label: 'Editor', value: 'e' },
* ] }
* onChange={ ( value ) => setOption( value ) }
* />
* );
* };
* ```
*/
export function RadioControl(
// ref is omitted until we have `WordPressComponentPropsWithoutRef` or add
// ref forwarding to RadioControl.
props: Omit<
WordPressComponentProps< RadioControlProps, 'input', false >,
'ref'
>
function UnforwardedRadioControl(
props: WordPressComponentProps< RadioControlProps, 'input', false >,
forwardedRef: ForwardedRef< any >
) {
const {
label,
Expand Down Expand Up @@ -93,6 +65,7 @@ export function RadioControl(
aria-describedby={
!! help ? `${ id }__help` : undefined
}
ref={ forwardedRef }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Havn't used forwardRef inside an Array.map before. Anything we need to think about or is this pattern ok?

Copy link
Contributor Author

@ciampo ciampo Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — I had actually missed that map statement while making these changes quickly.

It turns out that this task is more complicated than expected — we could forward the ref to BaseControl, but BaseControl currently doesn't accept refs.

I tried to quickly add ref forwarding to BaseControl, but I ran into type issues — mainly to do with the fact that VisualLabel is currently defined as a static property on BaseControl (e.g. BaseControl.VisualLabel), and adding ref forwarding causes an error with that

Screenshot 2022-06-10 at 09 19 08

Since this is not currently a priority, I've decided to stop investigating this further for now. What do folks think is the best way forward? I'm also happy to drop this entirely for the time being.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we can drop it for now, unless we have a concrete use case. I came across this note in the React docs that forwarding refs is a breaking change, and while I'm not 100% sure about what that entails, it did give me pause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will close this PR for now

{ ...additionalProps }
/>
<label htmlFor={ `${ id }-${ index }` }>
Expand All @@ -104,4 +77,31 @@ export function RadioControl(
);
}

/**
* Render a user interface to select the user type using radio inputs.
*
* ```jsx
* import { RadioControl } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* const MyRadioControl = () => {
* const [ option, setOption ] = useState( 'a' );
*
* return (
* <RadioControl
* label="User type"
* help="The type of the current user"
* selected={ option }
* options={ [
* { label: 'Author', value: 'a' },
* { label: 'Editor', value: 'e' },
* ] }
* onChange={ ( value ) => setOption( value ) }
* />
* );
* };
* ```
*/
export const RadioControl = forwardRef( UnforwardedRadioControl );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ciampo Does that function name seem confusing? UnforwardedRadioControl? Not sure if other components use that but it just seems weird especially reading from start to finish as the function now forwards the ref.

Copy link
Contributor Author

@ciampo ciampo Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pattern that @mirka and I have adopted in the context of the TypeScript migration that we're undertaking across the components in the package (see this guide for more details)

In order to have Storybook extract and show prop types correctly, we need to have a named export for the component ("RadioControl" in this case). Therefore, we had to find a suitable name for the intermediate representation of the component, and we opted for:

  • adding the "unforwarded" prefix when the component is later wrapped in a forwardRef call
  • adding the "unconnected" prefix when the component is later wrapped in a contextConnect call

Hope that's a bit more clear!


export default RadioControl;