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.

Sign up for GitHub

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

Allow define RadioControl label with ReactNode component #66615

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from
Open

Allow define RadioControl label with ReactNode component #66615

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
840ec87
radio control label accepts ReactNode type
retrofox Oct 30, 2024
b62bf45
add With Component Labels story
retrofox Oct 30, 2024
11d2f7e
update radio control Readme doc
retrofox Oct 30, 2024
acb4f66
add title to story labels
retrofox Oct 30, 2024
bfbfb32
Update packages/components/src/radio-control/README.md
retrofox Nov 27, 2024
7a5bd2f
Update packages/components/src/radio-control/types.ts
retrofox Nov 27, 2024
5d175c8
Update packages/components/src/radio-control/types.ts
retrofox Nov 27, 2024
b9a85c0
simplify RadioControl with component label story
retrofox Nov 27, 2024
c6b9c9e
improve the label TS type definition
retrofox Nov 27, 2024
3c254e2
Update packages/components/src/radio-control/README.md
retrofox Dec 17, 2024
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
2 changes: 1 addition & 1 deletion packages/components/src/radio-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ A function that receives the value of the new option that is being selected as i

An array of objects containing the value and label of the options.

- `label`: `string` The label to be shown to the user.
- `label`: `string` | `ReactNode` The label to be shown to the user.
- `value`: `string` The internal value compared against select and passed to onChange.

* Required: No
Expand Down
53 changes: 53 additions & 0 deletions packages/components/src/radio-control/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useState } from '@wordpress/element';
* Internal dependencies
*/
import RadioControl from '..';
import { Path, SVG } from '@wordpress/primitives';

const meta: Meta< typeof RadioControl > = {
component: RadioControl,
Expand Down Expand Up @@ -91,3 +92,55 @@ WithOptionDescriptions.args = {
},
],
};

export const WithComponentLabels: StoryFn< typeof RadioControl > =
Template.bind( {} );

function Rating( { stars, ariaLabel }: { stars: number; ariaLabel: string } ) {
const starPath =
'M12 17.27L18.18 21l-1.64-7.03L22 9.24l-7.19-.61L12 2 9.19 8.63 2 9.24l5.46 4.73L5.82 21z';
return (
<div
style={ { display: 'flex' } }
aria-label={ ariaLabel }
title={ ariaLabel }
>
{ Array.from( { length: stars }, ( _, index ) => (
<SVG
key={ index }
width={ 24 }
height={ 24 }
viewBox="0 0 24 24"
>
<Path d={ starPath } />
</SVG>
) ) }
</div>
);
}

WithComponentLabels.args = {
label: 'Rating',
options: [
{
label: <Rating stars={ 5 } ariaLabel="Five Stars" />,
value: '5',
},
{
label: <Rating stars={ 4 } ariaLabel="Four Stars" />,
value: '4',
},
{
label: <Rating stars={ 3 } ariaLabel="Three Stars" />,
value: '3',
},
{
label: <Rating stars={ 2 } ariaLabel="Two Stars" />,
value: '2',
},
{
label: <Rating stars={ 1 } ariaLabel="One Star" />,
value: '1',
},
],
};
4 changes: 2 additions & 2 deletions packages/components/src/radio-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ export type RadioControlProps = Pick<
*/
options?: {
/**
* The label to be shown to the user
* The label to be shown to the user.
*/
label: string;
label: string | React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

ReactNode is a bit broad. Do we really want label to possibly be boolean or null or even number? I wonder if we meant ReactElement instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, I don't think so. Maybe a number, but I believe string covers that.
Replacing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c6b9c9e

Copy link
Member

Choose a reason for hiding this comment

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

Do we really want label to possibly be boolean or null

This is a reasonable point, but I think we should also consider how many of our components that accept non-string labels right now already accept ReactNode. Introducing a subtle inconsistency like this could cause complications when composing components/types. Would it be ok to consider a package-wide switch from ReactNode to ReactElement as a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to consider this a separate issue 👍 Let's document it in an issue though.

/**
* The internal value compared against select and passed to onChange
*/
Expand Down
Loading