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

Jump to bottom

feat: improve RadioButton and RadioButtonGroup types #16648

Merged
merged 1 commit into from
Jun 12, 2024
Merged

feat: improve RadioButton and RadioButtonGroup types #16648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
149 changes: 77 additions & 72 deletions packages/react/src/components/RadioButton/RadioButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ export interface RadioButtonProps
* the underlying `<input>` changes
*/
onChange?: (
value: string | number,
name: string | undefined,
value: RadioButtonProps['value'],
name: RadioButtonProps['name'],
Comment on lines +75 to +76
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can directly reference other types within the same interface when the expected value is the same (e.g., props.name and props.value are what we're passing to the onChange handler).

Referencing types this way provides more context and reduces the work later, should RadioButton types change in the future.

This is also how it was originally typed (See DefinitelyTyped)

event: React.ChangeEvent<HTMLInputElement>
) => void;

Expand All @@ -98,80 +98,85 @@ export interface RadioButtonProps
required?: boolean;
}

const RadioButton = React.forwardRef((props: RadioButtonProps, ref) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref is now correctly typed.

Most of the following diff below is due to prettier's formatting adjusting the indentation. I recommend viewing with ⚙️ whitespace changes hidden.

const {
className,
disabled,
hideLabel,
id,
labelPosition = 'right',
labelText = '',
name,
onChange = () => {},
value = '',
slug,
required,
...rest
} = props;

const prefix = usePrefix();
const uid = useId('radio-button');
const uniqueId = id || uid;

function handleOnChange(event) {
onChange(value, name, event);
}

const innerLabelClasses = classNames(`${prefix}--radio-button__label-text`, {
[`${prefix}--visually-hidden`]: hideLabel,
});

const wrapperClasses = classNames(
className,
`${prefix}--radio-button-wrapper`,
{
[`${prefix}--radio-button-wrapper--label-${labelPosition}`]:
labelPosition !== 'right',
[`${prefix}--radio-button-wrapper--slug`]: slug,
const RadioButton = React.forwardRef<HTMLInputElement, RadioButtonProps>(
(props, ref) => {
const {
className,
disabled,
hideLabel,
id,
labelPosition = 'right',
labelText = '',
name,
onChange = () => {},
value = '',
slug,
required,
...rest
} = props;

const prefix = usePrefix();
const uid = useId('radio-button');
const uniqueId = id || uid;

function handleOnChange(event: React.ChangeEvent<HTMLInputElement>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a bonus, I added some missing types while working in these files.

onChange(value, name, event);
}
);

const inputRef = useRef<HTMLInputElement>(null);
const innerLabelClasses = classNames(
`${prefix}--radio-button__label-text`,
{
[`${prefix}--visually-hidden`]: hideLabel,
}
);
Comment on lines +126 to +131
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an automatic prettier adjustment.


const wrapperClasses = classNames(
className,
`${prefix}--radio-button-wrapper`,
{
[`${prefix}--radio-button-wrapper--label-${labelPosition}`]:
labelPosition !== 'right',
[`${prefix}--radio-button-wrapper--slug`]: slug,
}
);

const inputRef = useRef<HTMLInputElement>(null);

let normalizedSlug: React.ReactElement | undefined;
if (slug && React.isValidElement(slug)) {
const size = slug.props?.['kind'] === 'inline' ? 'md' : 'mini';
normalizedSlug = React.cloneElement(slug as React.ReactElement<any>, {
size,
});
}

let normalizedSlug;
if (slug && React.isValidElement(slug)) {
const size = slug.props?.['kind'] === 'inline' ? 'md' : 'mini';
normalizedSlug = React.cloneElement(slug as React.ReactElement<any>, {
size,
});
return (
<div className={wrapperClasses}>
<input
{...rest}
type="radio"
className={`${prefix}--radio-button`}
onChange={handleOnChange}
id={uniqueId}
ref={mergeRefs(inputRef, ref)}
disabled={disabled}
value={value}
name={name}
required={required}
/>
<label htmlFor={uniqueId} className={`${prefix}--radio-button__label`}>
<span className={`${prefix}--radio-button__appearance`} />
{labelText && (
<Text className={innerLabelClasses}>
{labelText}
{normalizedSlug}
</Text>
)}
</label>
</div>
);
}

return (
<div className={wrapperClasses}>
<input
{...rest}
type="radio"
className={`${prefix}--radio-button`}
onChange={handleOnChange}
id={uniqueId}
ref={mergeRefs(inputRef, ref)}
disabled={disabled}
value={value}
name={name}
required={required}
/>
<label htmlFor={uniqueId} className={`${prefix}--radio-button__label`}>
<span className={`${prefix}--radio-button__appearance`} />
{labelText && (
<Text className={innerLabelClasses}>
{labelText}
{normalizedSlug}
</Text>
)}
</label>
</div>
);
});
);

RadioButton.displayName = 'RadioButton';

Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/components/RadioButton/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* LICENSE file in the root directory of this source tree.
*/

import RadioButton from './RadioButton';
import RadioButton, { RadioButtonProps } from './RadioButton';

export default RadioButton;
export { RadioButton };

export type { RadioButtonProps };
59 changes: 35 additions & 24 deletions packages/react/src/components/RadioButtonGroup/RadioButtonGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import React, {
useState,
} from 'react';
import classNames from 'classnames';
import type { RadioButtonProps } from '../RadioButton';
import { Legend } from '../Text';
import { usePrefix } from '../../internal/usePrefix';
import { WarningFilled, WarningAltFilled } from '@carbon/icons-react';
Expand Down Expand Up @@ -44,7 +45,7 @@ export interface RadioButtonGroupProps
/**
* Specify the `<RadioButton>` to be selected by default
*/
defaultSelected?: string | number;
defaultSelected?: RadioButtonProps['value'];

/**
* Specify whether the group is disabled
Expand Down Expand Up @@ -87,10 +88,11 @@ export interface RadioButtonGroupProps
* the group changes
*/
onChange?: (
selection: React.ReactNode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The selection will be the same as the RadioButton value. ReactNode is too broad.

name: string,
selection: RadioButtonProps['value'],
name: RadioButtonGroupProps['name'],
event: React.ChangeEvent<HTMLInputElement>
) => void;

/**
* Provide where radio buttons should be placed
*/
Expand Down Expand Up @@ -119,7 +121,8 @@ export interface RadioButtonGroupProps
/**
* Specify the value that is currently selected in the group
*/
valueSelected?: string | number;
valueSelected?: RadioButtonProps['value'];

/**
* `true` to specify if input selection in group is required.
*/
Expand Down Expand Up @@ -166,30 +169,38 @@ const RadioButtonGroup = React.forwardRef(
}

function getRadioButtons() {
const mappedChildren = React.Children.map(children, (radioButton) => {
const { value } = (radioButton as ReactElement)?.props ?? undefined;

const newProps = {
name: name,
key: value,
value: value,
onChange: handleOnChange,
checked: value === selected,
required: required,
};

if (!selected && (radioButton as ReactElement)?.props.checked) {
newProps.checked = true;
const mappedChildren = React.Children.map(
children as ReactElement<RadioButtonProps>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

children is now typed as a RadioButton allowing us to inherit the types from RadioButtonProps. Previously, all prop values referenced in this function were typed as any.

This way, we no longer need to keep asserting the type of radioButton.

(radioButton) => {
if (!radioButton) {
return;
}
Comment on lines +175 to +177
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The null-check from 🔴 184 was moved up here as an early return.


const newProps = {
name: name,
key: radioButton.props.value,
value: radioButton.props.value,
onChange: handleOnChange,
checked: radioButton.props.value === selected,
required: required,
};

if (!selected && radioButton.props.checked) {
newProps.checked = true;
}

return React.cloneElement(radioButton, newProps);
}
if (radioButton) {
return React.cloneElement(radioButton as ReactElement, newProps);
}
});
);

return mappedChildren;
}

function handleOnChange(newSelection, value, evt) {
function handleOnChange(
newSelection: RadioButtonProps['value'],
value: RadioButtonProps['name'],
evt: React.ChangeEvent<HTMLInputElement>
) {
if (!readOnly) {
if (newSelection !== selected) {
setSelected(newSelection);
Expand Down Expand Up @@ -230,7 +241,7 @@ const RadioButtonGroup = React.forwardRef(
const divRef = useRef<HTMLDivElement>(null);

// Slug is always size `mini`
let normalizedSlug;
let normalizedSlug: ReactElement | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done across all components for Type Safety and Clarity

if (slug && slug['type']?.displayName === 'Slug') {
normalizedSlug = React.cloneElement(slug as React.ReactElement<any>, {
size: 'mini',
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/components/RadioButtonGroup/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* LICENSE file in the root directory of this source tree.
*/

import RadioButtonGroup from './RadioButtonGroup';
import RadioButtonGroup, { RadioButtonGroupProps } from './RadioButtonGroup';

export default RadioButtonGroup;
export { RadioButtonGroup };

export type { RadioButtonGroupProps };
Loading