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

Consume core Penta updates: Form controls, checkbox, radio #10016

Merged
merged 8 commits into from
Mar 26, 2024
+133 −212
Merged

Consume core Penta updates: Form controls, checkbox, radio #10016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e4da81b
feat(FormGroup): add FormGroupLabelHelp and update examples to use it
adamviktora Jan 17, 2024
9faa8ea
docs(FormGroup): recommend using FormGroupLabelHelp
adamviktora Jan 17, 2024
5a8470b
test(FormGroup): update spaces in snapshots
adamviktora Jan 17, 2024
4c5f0b2
Updated labelIcon prop name
thatblindgeye Feb 8, 2024
a417946
Update prop in example
thatblindgeye Feb 8, 2024
8963d67
feat(Checkbox/Radio): replace isLabelBeforeButton flag with labelPosi…
adamviktora Feb 21, 2024
41b1c0c
test(Radio): update snapshot to v6
adamviktora Feb 22, 2024
6225579
docs(FormFieldGroupHeader): prop description
adamviktora Mar 12, 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
8 changes: 4 additions & 4 deletions packages/react-core/src/components/Checkbox/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ export interface CheckboxProps
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;
/** Flag to show if the checkbox selection is valid or invalid. */
isValid?: boolean;
/** Flag to show if the checkbox is disabled. */
Expand All @@ -31,6 +29,8 @@ export interface CheckboxProps
onChange?: (event: React.FormEvent<HTMLInputElement>, checked: boolean) => void;
/** Label text of the checkbox. */
label?: React.ReactNode;
/** Sets the position of the label. Defaults to 'end' (after the checkbox input). */
labelPosition?: 'start' | 'end';
/** Id of the checkbox. */
id: string;
/** Aria-label of the checkbox. */
Expand Down Expand Up @@ -85,7 +85,7 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
inputClassName,
onChange,
isLabelWrapped,
isLabelBeforeButton,
labelPosition = 'end',
isValid,
isDisabled,
isRequired,
Expand Down Expand Up @@ -156,7 +156,7 @@ class Checkbox extends React.Component<CheckboxProps, CheckboxState> {
className={css(styles.check, !label && styles.modifiers.standalone, className)}
htmlFor={wrapWithLabel ? props.id : undefined}
>
{isLabelBeforeButton ? (
{labelPosition === 'start' ? (
<>
{labelRendered}
{inputRendered}
Expand Down
4 changes: 2 additions & 2 deletions packages/react-core/src/components/Checkbox/__tests__/Checkbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,10 +263,10 @@
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'} />);
test('Renders label before checkbox input if labelPosition is "start"', () => {
render(<Checkbox id="test-id" labelPosition="start" label={'test checkbox label'} />);

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

Check warning on line 269 in packages/react-core/src/components/Checkbox/__tests__/Checkbox.test.tsx

View workflow job for this annotation

GitHub Actions / lint 

Forbidden non-null assertion

expect(wrapper.children[0].tagName).toBe('LABEL');
expect(wrapper.children[1].tagName).toBe('INPUT');
Expand Down
7 changes: 2 additions & 5 deletions packages/react-core/src/components/Form/FormFieldGroupHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,13 @@ import * as React from 'react';
import styles from '@patternfly/react-styles/css/components/Form/form';
import { css } from '@patternfly/react-styles';

// typo - rename to FormFieldGroupHeaderTitleTextObject during breaking change release
export interface FormFiledGroupHeaderTitleTextObject {
export interface FormFieldGroupHeaderTitleTextObject {
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 exported, do we have codemod issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! now we do: patternfly/pf-codemods#609

/** Title text. */
text: React.ReactNode;
/** The applied to the title div for accessibility */
/** The id applied to the title div for accessibility */
id: string;
}

export interface FormFieldGroupHeaderTitleTextObject extends FormFiledGroupHeaderTitleTextObject {}

export interface FormFieldGroupHeaderProps extends React.HTMLProps<HTMLDivElement> {
/** Additional classes added to the section */
className?: string;
Expand Down
15 changes: 8 additions & 7 deletions packages/react-core/src/components/Form/FormGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ export interface FormGroupProps extends Omit<React.HTMLProps<HTMLDivElement>, 'l
label?: React.ReactNode;
/** Additional label information displayed after the label. */
labelInfo?: React.ReactNode;
/** Sets an icon for the label. For providing additional context. Host element for Popover */
labelIcon?: React.ReactElement;
/** A help button for the label. We recommend using FormGroupLabelHelp element as a help icon button. The help button should be wrapped or linked to our popover component. */
labelHelp?: React.ReactElement;
/** Sets the FormGroup required. */
isRequired?: boolean;
/** Sets the FormGroup isInline. */
Expand All @@ -38,7 +38,7 @@ export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
className = '',
label,
labelInfo,
labelIcon,
labelHelp,
isRequired = false,
isInline = false,
hasNoPaddingTop = false,
Expand All @@ -51,7 +51,7 @@ export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
const LabelComponent = isGroupOrRadioGroup ? 'span' : 'label';

const labelContent = (
<React.Fragment>
<>
<LabelComponent className={css(styles.formLabel)} {...(!isGroupOrRadioGroup && { htmlFor: fieldId })}>
<span className={css(styles.formLabelText)}>{label}</span>
{isRequired && (
Expand All @@ -60,9 +60,10 @@ export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
{ASTERISK}
</span>
)}
</LabelComponent>{' '}
{React.isValidElement(labelIcon) && labelIcon}
</React.Fragment>
</LabelComponent>
<>&nbsp;&nbsp;</>
{React.isValidElement(labelHelp) && <span className={styles.formGroupLabelHelp}>{labelHelp}</span>}
</>
);

return (
Expand Down
24 changes: 24 additions & 0 deletions packages/react-core/src/components/Form/FormGroupLabelHelp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import { Button, ButtonProps } from '../Button';
import QuestionCircleIcon from '@patternfly/react-icons/dist/esm/icons/question-circle-icon';

/** A help button to be passed to the FormGroup's labelHelp property. This should be wrapped or linked
* to our Popover component.
*/
export interface FormGroupLabelHelpProps extends ButtonProps {
/** Adds an accessible name for the help button. */
'aria-label': string;
/** Additional classes added to the help button. */
className?: string;
}

export const FormGroupLabelHelp: React.FunctionComponent<FormGroupLabelHelpProps> = ({
'aria-label': ariaLabel,
className,
...props
}) => (
<Button aria-label={ariaLabel} className={className} variant="plain" hasNoPadding {...props}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just mention that in core we use the <span> version of the button component here to help with line wrapping, though IIRC I don't know that it will make any sort of difference on the react side.

The reason for ^ is that some users want the help icon to wrap with the label text - or more specifically, they don't want the help icon to wrap on its own line (orphan), so it needs to wrap with at least one word of the label text. We can't get the label text to wrap with a <button>, but it will wrap with a <span>. However, another thing we can't get to wrap with text is an <svg> - which is how react-icons' icons are delivered. So even if we make the button a <span>, since react uses <svg>'s for the icons, it won't wrap with the text anyways.

I might suggest using the <span> anyways, if there are no real downsides to using that in favor of a <button>, on the off chance that we're able to figure out how to get an <svg> to wrap with text. That would also allow a user to pass a non-svg as the icon if we ever added that support - doesn't look like it's supported now.

Copy link
Contributor Author

@adamviktora adamviktora Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mcoker Good point Michael!

I remember I actually was applying component="span" and isInline props before to match the core implementation (which in combination leads to these attributes: type="button" role="button" tabindex="0" being added on the <span> element, which is exactly what is in core).

So I was wondering why it is not there now.

I was just about to commit your suggestion and then I realized why I did not apply the component="span" back then. The reason is accessibility of the Popover. Although the tabindex="0" makes the span focusable, the Popover cannot be opened on pressing the enter key. It works only if the element is a <button>.

span:
https://github.com/patternfly/patternfly-react/assets/84135613/a452671a-9523-4a64-993f-89029e81c17f

button:
https://github.com/patternfly/patternfly-react/assets/84135613/534086d0-7319-4ada-aebc-5c0664510a99

Seems like this issue is related to the implementation of the Popover, so I am taking a look at that now.

For now I would keep it as <button>, so the Popover is accessible, and once we figure that out, we can add the component="span" and isInline props to match core.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamviktora nice, thanks for digging in and for the explanation! Sounds like a plan 👍👍

<QuestionCircleIcon />
</Button>
);
FormGroupLabelHelp.displayName = 'FormGroupLabelHelp';
14 changes: 7 additions & 7 deletions packages/react-core/src/components/Form/__tests__/__snapshots__/FormGroup.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ exports[`FormGroup should render correctly when label is not a string with Child
</div>
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control"
Expand Down Expand Up @@ -55,7 +55,7 @@ exports[`FormGroup should render default form group variant 1`] = `
label
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control"
Expand Down Expand Up @@ -88,7 +88,7 @@ exports[`FormGroup should render form group variant with node label 1`] = `
</h1>
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control"
Expand Down Expand Up @@ -125,7 +125,7 @@ exports[`FormGroup should render form group variant with required label 1`] = `
*
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control"
Expand Down Expand Up @@ -177,7 +177,7 @@ exports[`FormGroup should render form group with additonal label info 1`] = `
</h1>
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-label-info"
Expand Down Expand Up @@ -218,7 +218,7 @@ exports[`FormGroup should render horizontal form group variant 1`] = `
label
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control"
Expand Down Expand Up @@ -254,7 +254,7 @@ exports[`FormGroup should render stacked horizontal form group variant 1`] = `
label
</span>
</label>
  
</div>
<div
class="pf-v6-c-form__group-control pf-m-stack"
Expand Down
17 changes: 1 addition & 16 deletions packages/react-core/src/components/Form/examples/Form.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ propComponents:
'ActionGroup',
'Form',
'FormGroup',
'FormGroupLabelHelp',
'FormSection',
'FormHelperText',
'FormFieldGroup',
Expand All @@ -21,22 +22,6 @@ propComponents:
]
---

import {
Button,
Form,
FormGroup,
Popover,
TextInput,
TextArea,
FormSelect,
FormHelperText,
FormFieldGroup,
FormFieldGroupHeader,
FormFieldGroupExpandable,
Checkbox,
ActionGroup,
Radio
} from '@patternfly/react-core';
import ExclamationCircleIcon from '@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon';
import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
import TrashIcon from '@patternfly/react-icons/dist/esm/icons/trash-icon';
Expand Down
17 changes: 4 additions & 13 deletions packages/react-core/src/components/Form/examples/FormBasic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import {
Radio,
HelperText,
HelperTextItem,
FormHelperText
FormHelperText,
FormGroupLabelHelp
} from '@patternfly/react-core';
import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
import styles from '@patternfly/react-styles/css/components/Form/form';

export const FormBasic: React.FunctionComponent = () => {
const [name, setName] = React.useState('');
Expand All @@ -36,7 +35,7 @@ export const FormBasic: React.FunctionComponent = () => {
<Form>
<FormGroup
label="Full name"
labelIcon={
labelHelp={
<Popover
headerContent={
<div>
Expand Down Expand Up @@ -64,15 +63,7 @@ export const FormBasic: React.FunctionComponent = () => {
</div>
}
>
<button
type="button"
aria-label="More info for name field"
onClick={(e) => e.preventDefault()}
aria-describedby="simple-form-name-01"
className={styles.formGroupLabelHelp}
>
<HelpIcon />
</button>
<FormGroupLabelHelp aria-label="More info for name field" />
</Popover>
}
isRequired
Expand Down
17 changes: 4 additions & 13 deletions packages/react-core/src/components/Form/examples/FormGroupLabelInfo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ import {
Popover,
HelperText,
HelperTextItem,
FormHelperText
FormHelperText,
FormGroupLabelHelp
} from '@patternfly/react-core';
import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
import styles from '@patternfly/react-styles/css/components/Form/form';

export const FormGroupLabelInfo: React.FunctionComponent = () => {
const [name, setName] = React.useState('');
Expand All @@ -23,7 +22,7 @@ export const FormGroupLabelInfo: React.FunctionComponent = () => {
<FormGroup
label="Full name"
labelInfo="Additional label info"
labelIcon={
labelHelp={
<Popover
headerContent={
<div>
Expand Down Expand Up @@ -51,15 +50,7 @@ export const FormGroupLabelInfo: React.FunctionComponent = () => {
</div>
}
>
<button
type="button"
aria-label="More info for name field"
onClick={(e) => e.preventDefault()}
aria-describedby="form-group-label-info"
className={styles.formGroupLabelHelp}
>
<HelpIcon />
</button>
<FormGroupLabelHelp aria-label="More info for name field" />
</Popover>
}
isRequired
Expand Down
17 changes: 4 additions & 13 deletions packages/react-core/src/components/Form/examples/FormLimitWidth.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import {
Radio,
HelperText,
HelperTextItem,
FormHelperText
FormHelperText,
FormGroupLabelHelp
} from '@patternfly/react-core';
import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
import styles from '@patternfly/react-styles/css/components/Form/form';

export const FormLimitWidth: React.FunctionComponent = () => {
const [name, setName] = React.useState('');
Expand All @@ -36,7 +35,7 @@ export const FormLimitWidth: React.FunctionComponent = () => {
<Form isWidthLimited>
<FormGroup
label="Full name"
labelIcon={
labelHelp={
<Popover
headerContent={
<div>
Expand Down Expand Up @@ -64,15 +63,7 @@ export const FormLimitWidth: React.FunctionComponent = () => {
</div>
}
>
<button
type="button"
aria-label="More info for name field"
onClick={(e) => e.preventDefault()}
aria-describedby="simple-form-name-02"
className={styles.formGroupLabelHelp}
>
<HelpIcon />
</button>
<FormGroupLabelHelp aria-label="More info for name field" />
</Popover>
}
isRequired
Expand Down
1 change: 1 addition & 0 deletions packages/react-core/src/components/Form/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export * from './FormFieldGroup';
export * from './FormFieldGroupExpandable';
export * from './FormFieldGroupHeader';
export * from './FormGroup';
export * from './FormGroupLabelHelp';
export * from './FormHelperText';
export * from './FormSection';
export * from './FormContext';
Loading
Loading