Skip to content

Flexible alert actions #3822

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

Closed
seanforyou23 wants to merge 1 commit into from
Closed

Flexible alert actions #3822

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
15 changes: 13 additions & 2 deletions packages/react-core/src/components/Alert/Alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,23 @@ export enum AlertVariant {
default = 'default'
}

export type AlertActionRenderer = ({
title,
variantLabel
}: {
title: React.ReactNode;
variantLabel: string;
}) => React.ReactNode;

export interface AlertProps extends Omit<React.HTMLProps<HTMLDivElement>, 'action' | 'title'> {
/** Adds Alert variant styles */
variant?: 'success' | 'danger' | 'warning' | 'info' | 'default';
/** Flag to indicate if the Alert is inline */
isInline?: boolean;
/** Title of the Alert */
title: React.ReactNode;
/** Action button to put in the Alert. Should be <AlertActionLink> or <AlertActionCloseButton> */
action?: React.ReactNode;
/** Action button to put in the Alert. Often <AlertActionLink> or <AlertActionCloseButton> */
action?: AlertActionRenderer | React.ReactNode;
Copy link
Contributor

@redallen redallen Apr 13, 2020

Choose a reason for hiding this comment

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

Isn't this really React.FunctionComponent<{ title: React.ReactNode, variantLabel: string }>?

/** Content rendered inside the Alert */
children?: React.ReactNode;
/** Additional classes added to the Alert */
Expand Down Expand Up @@ -80,6 +88,9 @@ export const Alert: React.FunctionComponent<AlertProps & OUIAProps> = ({
{action && (typeof action === 'object' || typeof action === 'string') && (
Copy link
Contributor

@redallen redallen Apr 13, 2020

Choose a reason for hiding this comment

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

Perhaps a better API would be to leave action's type as React.ReactNode and use React.cloneElement to add the title and variantLabel props? This way class components could be passed as well, and we don't have to amend the API.

{React.isValidElement(action) && (
  <div className={css(styles.alertAction)}>
    {React.cloneElement(action as React.ReactElement, {
      title,
      variantLabel
    })}
  </div>
)}

Consumers would then be able to use added title and variantLabel props like this:

<Alert action={({title, variantLabel}) => <div>{title}{variantLabel}</div>} />

OR like this:

class MyAlertAction extends React.Component {
  render() {
    return <div>{this.props.title}{this.props.variantLabel}</div>;
  }
}

<Alert action={<MyAction />} />

<div className={css(styles.alertAction)}>{action}</div>
)}
{action && typeof action === 'function' && (
<div className={css(styles.alertAction)}>{action({ title, variantLabel })}</div>
Copy link
Collaborator

@jschuler jschuler Apr 13, 2020

Choose a reason for hiding this comment

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

I don't understand, couldn't this be
<div className={css(styles.alertAction)}>{action}</div> and since consumer is passing in title and variantLabel as props they already have knowledge of the props so aren't we just giving it back to them?

)}
</AlertContext.Provider>
</div>
);
Expand Down
5 changes: 1 addition & 4 deletions packages/react-core/src/components/Alert/AlertActionCloseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,21 @@ interface AlertActionCloseButtonProps extends ButtonProps {
onClose?: () => void;
/** Aria Label for the Close button */
'aria-label'?: string;
/** Variant Label for the Close button */
variantLabel?: string;
}

export const AlertActionCloseButton = ({
// eslint-disable-next-line @typescript-eslint/no-unused-vars
className = '',
onClose = () => undefined as any,
'aria-label': ariaLabel = '',
variantLabel,
...props
}: AlertActionCloseButtonProps) => (
<AlertContext.Consumer>
{({ title, variantLabel: alertVariantLabel }) => (
<Button
variant={ButtonVariant.plain}
onClick={onClose}
aria-label={ariaLabel === '' ? `Close ${variantLabel || alertVariantLabel} alert: ${title}` : ariaLabel}
aria-label={ariaLabel === '' ? `Close ${alertVariantLabel} alert: ${title}` : ariaLabel}
{...props}
>
<TimesIcon />
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Alert/AlertActionLink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Button, ButtonVariant, ButtonProps } from '../Button';

export interface AlertActionLinkProps extends ButtonProps {
/** Content rendered inside the AlertLinkAction */
children?: string;
children: string;
Copy link
Collaborator

@jschuler jschuler Apr 13, 2020

Choose a reason for hiding this comment

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

Should be ReactNode so it could work with i18n libraries for example

/** Additional classes added to the AlertActionLink */
className?: string;
}
Expand Down
14 changes: 14 additions & 0 deletions packages/react-core/src/components/Alert/__tests__/Alert.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ Object.values(AlertVariant).forEach(variant => {
expect(view).toMatchSnapshot();
});

test('Custom action', () => {
const view = mount(
<Alert
variant={variant}
aria-label={`Custom aria label for ${variant}`}
action={({variantLabel, title}) => (<>{variantLabel} {title}</>)}
title="Some title"
>
Some alert
</Alert>
);
expect(view).toMatchSnapshot();
});

test('inline variation', () => {
const view = mount(
<Alert variant={variant} isInline title="Some title">
Expand Down
Loading