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

Add support for isLabelWrapped and component in Checkbox / Radio #9830

Merged
merged 8 commits into from
Jan 9, 2024
92 changes: 63 additions & 29 deletions packages/react-core/src/components/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,16 @@ import { ASTERISK } from '../../helpers/htmlConstants';
export interface CheckboxProps
extends Omit<React.HTMLProps<HTMLInputElement>, 'type' | 'onChange' | 'disabled' | 'label'>,
OUIAProps {
/** Additional classes added to the checkbox. */
/** Additional classes added to the checkbox wrapper. This wrapper will be div element by default. It will be a label element if
* isLabelWrapped is true, or it can be overridden by any element specified in the component prop.
*/
className?: string;
/** Additional classed added to the radio input */
/** Additional classes added to the checkbox input. */
inputClassName?: string;
/** Flag to indicate whether the checkbox wrapper element is a <label> element for the checkbox input. Will not apply if a component prop (with a value other than a "label") is specified. */
isLabelWrapped?: boolean;
/** Flag to show if the checkbox label is shown before the checkbox input. */
isLabelBeforeButton?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but I wonder if in a breaking change we should rename this and the radio to remove the "button" part and use something more generic. isLabelBeforeInput would probably be more accurate, but it could just be isLabelStart or something along those lines - do we have any kind of pattern for naming props like this? This prop is a boolean, but the button and accordion have props iconPosition="[start/end]" and togglePosition=[start/end] that do something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the core docs and it has a "Reversed" example, so possibly isReversed or isLabelReversed?

Screenshot 2024-01-09 at 11 11 12

I wonder whether we rename it now to have less codemods, or just keep it consistent in v5 so we keep the isLabelBeforeButton to match the Radio prop.

/** Flag to show if the checkbox selection is valid or invalid. */
isValid?: boolean;
/** Flag to show if the checkbox is disabled. */
Expand All @@ -33,7 +39,7 @@ export interface CheckboxProps
description?: React.ReactNode;
/** Body text of the checkbox */
body?: React.ReactNode;
/** Sets the input wrapper component to render. Defaults to <div> */
/** Sets the checkbox wrapper component to render. Defaults to "div". If set to "label", behaves the same as if isLabelWrapped prop was specified. */
component?: React.ElementType;
/** Value to overwrite the randomly generated data-ouia-component-id.*/
ouiaId?: number | string;
Expand All @@ -52,13 +58,13 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
static displayName = 'Checkbox';
static defaultProps: PickOptional<CheckboxProps> = {
className: '',
isLabelWrapped: false,
isValid: true,
isDisabled: false,
isRequired: false,
isChecked: false,
onChange: defaultOnChange,
ouiaSafe: true,
component: 'div'
ouiaSafe: true
};

constructor(props: any) {
Expand All @@ -78,6 +84,8 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
className,
inputClassName,
onChange,
isLabelWrapped,
isLabelBeforeButton,
isValid,
isDisabled,
isRequired,
Expand All @@ -89,7 +97,7 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
body,
ouiaId,
ouiaSafe,
component: Component,
component,
...props
} = this.props;
if (!props.id) {
Expand All @@ -107,31 +115,57 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
checkedProps.defaultChecked = defaultChecked;
}

const inputRendered = (
<input
{...props}
className={css(styles.checkInput, inputClassName)}
type="checkbox"
onChange={this.handleChange}
aria-invalid={!isValid}
aria-label={ariaLabel}
disabled={isDisabled}
required={isRequired}
ref={(elem) => elem && (elem.indeterminate = isChecked === null)}
{...checkedProps}
{...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : this.state.ouiaStateId, ouiaSafe)}
/>
);

const wrapWithLabel = (isLabelWrapped && !component) || component === 'label';

const Label = wrapWithLabel ? 'span' : 'label';
const labelRendered = label ? (
<Label
className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)}
htmlFor={!wrapWithLabel && props.id}
>
{label}
{isRequired && (
<span className={css(styles.checkLabelRequired)} aria-hidden="true">
{ASTERISK}
</span>
)}
</Label>
) : null;

const Component = component ?? (wrapWithLabel ? 'label' : 'div');

checkedProps.checked = checkedProps.checked === null ? false : checkedProps.checked;
return (
<Component className={css(styles.check, !label && styles.modifiers.standalone, className)}>
<input
{...props}
className={css(styles.checkInput, inputClassName)}
type="checkbox"
onChange={this.handleChange}
aria-invalid={!isValid}
aria-label={ariaLabel}
disabled={isDisabled}
required={isRequired}
ref={(elem) => elem && (elem.indeterminate = isChecked === null)}
{...checkedProps}
{...getOUIAProps(Checkbox.displayName, ouiaId !== undefined ? ouiaId : this.state.ouiaStateId, ouiaSafe)}
/>
{label && (
<label className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} htmlFor={props.id}>
{label}
{isRequired && (
<span className={css(styles.checkLabelRequired)} aria-hidden="true">
{ASTERISK}
</span>
)}
</label>
<Component
className={css(styles.check, !label && styles.modifiers.standalone, className)}
htmlFor={wrapWithLabel && props.id}
>
{isLabelBeforeButton ? (
<>
{labelRendered}
{inputRendered}
</>
) : (
<>
{inputRendered}
{labelRendered}
</>
)}
{description && <span className={css(styles.checkDescription)}>{description}</span>}
{body && <span className={css(styles.checkBody)}>{body}</span>}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,47 @@ test('Renders with the provided component', () => {
expect(screen.getByRole('checkbox').parentElement?.tagName).toBe('SPAN');
});

test('Renders with the label wrapper if isLabelWrapped is provided', () => {
render(<Checkbox id="test-id" isLabelWrapped />);

expect(screen.getByRole('checkbox').parentElement?.tagName).toBe('LABEL');
});

test('Renders with span element around the inner label text if isLabelWrapped is provided', () => {
const labelText = "test checkbox label";
render(<Checkbox id="test-id" isLabelWrapped label={labelText} />);

expect(screen.getByText(labelText).tagName).toBe('SPAN');
});

test('Renders with the provided component although isLabelWrapped is provided', () => {
render(<Checkbox id="test-id" isLabelWrapped component="h3" />);

expect(screen.getByRole('checkbox').parentElement?.tagName).toBe('H3');
});

test('Renders with the label wrapper if component is set to label', () => {
render(<Checkbox id="test-id" component="label" />);

expect(screen.getByRole('checkbox').parentElement?.tagName).toBe('LABEL');
});

test('Renders with span element around the inner label text if component is set to label', () => {
const labelText = "test checkbox label";
render(<Checkbox id="test-id" component="label" label={labelText} />);

expect(screen.getByText(labelText).tagName).toBe('SPAN');
});

test('Renders label before checkbox input if isLabelBeforeButton is provided', () => {
render(<Checkbox id="test-id" isLabelBeforeButton label={"test checkbox label"} />);

const wrapper = screen.getByRole('checkbox').parentElement!;

expect(wrapper.children[0].tagName).toBe('LABEL');
expect(wrapper.children[1].tagName).toBe('INPUT');
});

test(`Spreads additional props`, () => {
render(<Checkbox id="test-id" data-testid="test-id" />);

Expand Down
77 changes: 39 additions & 38 deletions packages/react-core/src/components/Radio/Radio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@ import { getOUIAProps, OUIAProps, getDefaultOUIAId } from '../../helpers';
export interface RadioProps
extends Omit<React.HTMLProps<HTMLInputElement>, 'disabled' | 'label' | 'onChange' | 'type'>,
OUIAProps {
/** Additional classes added to the radio wrapper. This will be a div element if
* isLabelWrapped is true, otherwise this will be a label element.
/** Additional classes added to the radio wrapper. This wrapper will be div element by default. It will be a label element if
* isLabelWrapped is true, or it can be overridden by any element specified in the component prop.
*/
className?: string;
/** Additional classed added to the radio input */
/** Additional classes added to the radio input. */
inputClassName?: string;
/** Id of the radio. */
id: string;
/** Flag to show if the radio label is wrapped on small screen. */
/** Flag to indicate whether the radio wrapper element is a native label element for the radio input. Will not apply if a component prop (with a value other than a "label") is specified. */
isLabelWrapped?: boolean;
/** Flag to show if the radio label is shown before the radio button. */
/** Flag to show if the radio label is shown before the radio input. */
isLabelBeforeButton?: boolean;
/** Flag to show if the radio is checked. */
checked?: boolean;
Expand All @@ -39,6 +39,8 @@ export interface RadioProps
description?: React.ReactNode;
/** Body of the radio. */
body?: React.ReactNode;
/** Sets the radio wrapper component to render. Defaults to "div". If set to "label", behaves the same as if isLabelWrapped prop was specified. */
component?: React.ElementType;
/** Value to overwrite the randomly generated data-ouia-component-id.*/
ouiaId?: number | string;
/** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */
Expand Down Expand Up @@ -88,6 +90,7 @@ class Radio extends React.Component<RadioProps, { ouiaStateId: string }> {
body,
ouiaId,
ouiaSafe = true,
component,
...props
} = this.props;
if (!props.id) {
Expand All @@ -110,41 +113,39 @@ class Radio extends React.Component<RadioProps, { ouiaStateId: string }> {
/>
);

let labelRendered: React.ReactNode = null;
if (label && isLabelWrapped) {
labelRendered = <span className={css(styles.radioLabel, isDisabled && styles.modifiers.disabled)}>{label}</span>;
} else if (label) {
labelRendered = (
<label className={css(styles.radioLabel, isDisabled && styles.modifiers.disabled)} htmlFor={props.id}>
{label}
</label>
);
}
const wrapWithLabel = (isLabelWrapped && !component) || component === 'label';

const descRender = description ? <span className={css(styles.radioDescription)}>{description}</span> : null;
const bodyRender = body ? <span className={css(styles.radioBody)}>{body}</span> : null;
const childrenRendered = isLabelBeforeButton ? (
<>
{labelRendered}
{inputRendered}
{descRender}
{bodyRender}
</>
) : (
<>
{inputRendered}
{labelRendered}
{descRender}
{bodyRender}
</>
);
const Label = wrapWithLabel ? 'span' : 'label';
const labelRendered = label ? (
<Label
className={css(styles.radioLabel, isDisabled && styles.modifiers.disabled)}
htmlFor={!wrapWithLabel && props.id}
>
{label}
</Label>
) : null;

const Component = component ?? (wrapWithLabel ? 'label' : 'div');

return isLabelWrapped ? (
<label className={css(styles.radio, className)} htmlFor={props.id}>
{childrenRendered}
</label>
) : (
<div className={css(styles.radio, !label && styles.modifiers.standalone, className)}>{childrenRendered}</div>
return (
<Component
className={css(styles.radio, !label && styles.modifiers.standalone, className)}
htmlFor={wrapWithLabel && props.id}
>
{isLabelBeforeButton ? (
<>
{labelRendered}
{inputRendered}
</>
) : (
<>
{inputRendered}
{labelRendered}
</>
)}
{description && <span className={css(styles.radioDescription)}>{description}</span>}
{body && <span className={css(styles.radioBody)}>{body}</span>}
</Component>
);
}
}
Expand Down
41 changes: 41 additions & 0 deletions packages/react-core/src/components/Radio/__tests__/Radio.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,45 @@ describe('Radio', () => {

expect(myMock).toHaveBeenCalled();
});

test('Renders with the label wrapper if isLabelWrapped is provided', () => {
render(<Radio id="test-id" name="check" isLabelWrapped />);

expect(screen.getByRole('radio').parentElement?.tagName).toBe('LABEL');
});

test('Renders with span element around the inner label text if isLabelWrapped is provided', () => {
const labelText = 'test radio label';
render(<Radio id="test-id" name="check" isLabelWrapped label={labelText} />);

expect(screen.getByText(labelText).tagName).toBe('SPAN');
});

test('Renders with the provided component although isLabelWrapped is provided', () => {
render(<Radio id="test-id" name="check" isLabelWrapped component="h3" />);

expect(screen.getByRole('radio').parentElement?.tagName).toBe('H3');
});

test('Renders with the label wrapper if component is set to label', () => {
render(<Radio id="test-id" name="check" component="label" />);

expect(screen.getByRole('radio').parentElement?.tagName).toBe('LABEL');
});

test('Renders with span element around the inner label text if component is set to label', () => {
const labelText = 'test radio label';
render(<Radio id="test-id" name="check" component="label" label={labelText} />);

expect(screen.getByText(labelText).tagName).toBe('SPAN');
});

test('Renders label before radio input if isLabelBeforeButton is provided', () => {
render(<Radio id="test-id" name="check" isLabelBeforeButton label={"test radio label"} />);

const wrapper = screen.getByRole('radio').parentElement!;

expect(wrapper.children[0].tagName).toBe('LABEL');
expect(wrapper.children[1].tagName).toBe('INPUT');
});
});
Loading