Skip to content

feat(banner): add support for new styles in penta #9891

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

Merged
merged 3 commits into from
Dec 20, 2023
Merged

feat(banner): add support for new styles in penta #9891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f1ef5f
feat(banner): add support for new styles in penta
wise-king-sullyman Dec 13, 2023
d9529d6
chore(banner): update tests
wise-king-sullyman Dec 13, 2023
914ea1e
chore(banner): update banner documentation copy
wise-king-sullyman Dec 13, 2023
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
59 changes: 41 additions & 18 deletions packages/react-core/src/components/Banner/Banner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import * as React from 'react';
import styles from '@patternfly/react-styles/css/components/Banner/banner';
import { css } from '@patternfly/react-styles';

export type BannerColor = 'red' | 'orangered' | 'orange' | 'gold' | 'green' | 'cyan' | 'blue' | 'purple';

export type BannerStatus = 'success' | 'warning' | 'danger' | 'info' | 'custom';
Comment on lines +5 to +7
Copy link
Contributor Author

@wise-king-sullyman wise-king-sullyman Dec 13, 2023

Choose a reason for hiding this comment

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

Exported union types so that consumers can import them if needed, I remember that being talked about at an office hours not long ago.


export interface BannerProps extends React.HTMLProps<HTMLDivElement> {
/** Content rendered inside the banner. */
children?: React.ReactNode;
Expand All @@ -13,29 +17,48 @@ export interface BannerProps extends React.HTMLProps<HTMLDivElement> {
* be passed in when the banner conveys status/severity.
*/
screenReaderText?: string;
/** Variant styles for the banner. */
variant?: 'default' | 'blue' | 'red' | 'green' | 'gold';
/** Color options for the banner, will be overwritten by any applied using the status prop. */
color?: BannerColor;
/** Status style options for the banner, will overwrite any color applied using the color prop. */
status?: BannerStatus;
}
interface StatusBanner extends BannerProps {
color?: never;
status?: BannerStatus;
}

export const Banner: React.FunctionComponent<BannerProps> = ({
interface NonStatusBanner extends BannerProps {
color?: BannerColor;
status?: never;
}

export const Banner: React.FunctionComponent<StatusBanner | NonStatusBanner> = ({
Comment on lines +25 to +35
Copy link
Contributor Author

@wise-king-sullyman wise-king-sullyman Dec 13, 2023

Choose a reason for hiding this comment

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

Tried this out as a way of preventing both color and status props at the type level, I'm not committed to it if we don't like it though.

Copy link
Contributor

@thatblindgeye thatblindgeye Dec 18, 2023

Choose a reason for hiding this comment

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

Not entirely sure how I feel about this. It's definitely an improvement from what was attempted in 91d7907 (with the main issue being it didn't render props in the prop table correctly, whereas this PR does).

At the same time I'm almost wondering if it'd be better to just keep the variant prop, and makes its type BannerStatus | BannerColor?

Otherwise this looks good. If anything we could have a followup discussion about it since it's only going into the v6 branch so I wouldn't block over this going in now.

children,
className,
variant = 'default',
screenReaderText,
isSticky = false,
color,
status,
...props
}: BannerProps) => (
<div
className={css(
styles.banner,
styles.modifiers[variant as 'green' | 'red' | 'gold' | 'blue'],
isSticky && styles.modifiers.sticky,
className
)}
{...props}
>
{screenReaderText && <span className="pf-v5-screen-reader">{screenReaderText}</span>}
{children}
</div>
);
}: BannerProps) => {
const getStatusOrColorModifier = () => {
if (status) {
return styles.modifiers[status];
}

if (color) {
return styles.modifiers[color];
}
};

Copy link
Contributor

@gitdallas gitdallas Dec 15, 2023

Choose a reason for hiding this comment

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

not a big deal and maybe more of a preference thing, but as color and status are used exactly the same way, i think i'd just get rid of getStatusOrColorModifier and replace the call with (color || status) && styles.modifiers[color || status]

return (
<div
className={css(styles.banner, getStatusOrColorModifier(), isSticky && styles.modifiers.sticky, className)}
{...props}
>
{screenReaderText && <span className="pf-v5-screen-reader">{screenReaderText}</span>}
{children}
</div>
);
};
Banner.displayName = 'Banner';
67 changes: 56 additions & 11 deletions packages/react-core/src/components/Banner/__tests__/Banner.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,31 +27,76 @@ test('Renders with custom class name when className prop is provided', () => {
expect(screen.getByText('Test')).toHaveClass('custom-class');
});

test('Renders without any modifier class when variant prop is not passed', () => {
test('Renders without any modifier class when color and status props are not passed', () => {
render(<Banner>Test</Banner>);
expect(screen.getByText('Test')).toHaveClass(styles.banner, { exact: true });
});

test('Renders with class name pf-m-green when "green" is passed to variant prop', () => {
render(<Banner variant="green">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-green');
test('Renders with class name pf-m-red when "red" is passed to color prop', () => {
render(<Banner color="red">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-red');
});

test('Renders with class name pf-m-red when "red" is passed to variant prop', () => {
render(<Banner variant="red">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-red');
test('Renders with class name pf-m-orangered when "orangered" is passed to color prop', () => {
render(<Banner color="orangered">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-orangered');
});

test('Renders with class name pf-m-gold when "gold" is passed to variant prop', () => {
render(<Banner variant="gold">Test</Banner>);
test('Renders with class name pf-m-orange when "orange" is passed to color prop', () => {
render(<Banner color="orange">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-orange');
});

test('Renders with class name pf-m-gold when "gold" is passed to color prop', () => {
render(<Banner color="gold">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-gold');
});

test('Renders with class name pf-m-blue when "blue" is passed to variant prop', () => {
render(<Banner variant="blue">Test</Banner>);
test('Renders with class name pf-m-green when "green" is passed to color prop', () => {
render(<Banner color="green">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-green');
});

test('Renders with class name pf-m-cyan when "cyan" is passed to color prop', () => {
render(<Banner color="cyan">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-cyan');
});

test('Renders with class name pf-m-blue when "blue" is passed to color prop', () => {
render(<Banner color="blue">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-blue');
});

test('Renders with class name pf-m-purple when "purple" is passed to color prop', () => {
render(<Banner color="purple">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-purple');
});

test('Renders with class name pf-m-success when "success" is passed to status prop', () => {
render(<Banner status="success">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-success');
});

test('Renders with class name pf-m-warning when "warning" is passed to status prop', () => {
render(<Banner status="warning">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-warning');
});

test('Renders with class name pf-m-danger when "danger" is passed to status prop', () => {
render(<Banner status="danger">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-danger');
});

test('Renders with class name pf-m-info when "info" is passed to status prop', () => {
render(<Banner status="info">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-info');
});

test('Renders with class name pf-m-custom when "custom" is passed to status prop', () => {
render(<Banner status="custom">Test</Banner>);
expect(screen.getByText('Test')).toHaveClass('pf-m-custom');
});

test('Does not render pf-v5-screen-reader class by default', () => {
render(<Banner>Test</Banner>);
expect(screen.getByText('Test')).not.toContainHTML('<span class="pf-v5-screen-reader"></span>');
Expand Down
6 changes: 4 additions & 2 deletions packages/react-core/src/components/Banner/examples/Banner.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@ import BellIcon from '@patternfly/react-icons/dist/esm/icons/bell-icon';

### Basic

Banners can be styled with one of 5 different colors. A basic banner should only be used when the banner color does not represent status or severity.
Banners can be styled with one of 9 different colors using the `color` prop. A basic banner should only be used when the banner color does not represent status or severity.

```ts file="./BannerBasic.tsx"

```

### Status

When a banner is used to convey status, it is advised to pass in an icon inside the banner to convey the status in a way besides just color. The `screenReaderText` prop should also be passed in to convey the status/severity of the banner to users of certain assistive technologies such as screen readers.
When a banner is used to convey status it should be styled using the `status` prop. Additionally, it is advised to pass an icon inside the banner to convey the status in a way besides just color.

The `screenReaderText` prop should also be passed in to convey the status/severity of the banner to users of certain assistive technologies such as screen readers.

In the following example, a flex layout is used inside the banner content to show one possible way to create spacing between the icons and banner text.

Expand Down
16 changes: 12 additions & 4 deletions packages/react-core/src/components/Banner/examples/BannerBasic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,20 @@ export const BannerBasic: React.FunctionComponent = () => (
<>
<Banner>Default banner</Banner>
<br />
<Banner variant="blue">Blue banner</Banner>
<Banner color="red">Red banner</Banner>
<br />
<Banner variant="red">Red banner</Banner>
<Banner color="orangered">Orangered banner</Banner>
<br />
<Banner variant="green">Green banner</Banner>
<Banner color="orange">Orange banner</Banner>
<br />
<Banner variant="gold">Gold banner</Banner>
<Banner color="gold">Gold banner</Banner>
<br />
<Banner color="green">Green banner</Banner>
<br />
<Banner color="cyan">Cyan banner</Banner>
<br />
<Banner color="blue">Blue banner</Banner>
<br />
<Banner color="purple">Purple banner</Banner>
</>
);
26 changes: 13 additions & 13 deletions packages/react-core/src/components/Banner/examples/BannerStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,25 @@ import BellIcon from '@patternfly/react-icons/dist/esm/icons/bell-icon';

export const BannerStatus: React.FunctionComponent = () => (
<>
<Banner screenReaderText="Default banner">
<Banner screenReaderText="Success banner" status="success">
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
<BellIcon />
<CheckCircleIcon />
</FlexItem>
<FlexItem>Default banner</FlexItem>
<FlexItem>Success banner</FlexItem>
</Flex>
</Banner>
<br />
<Banner screenReaderText="Info banner" variant="blue">
<Banner screenReaderText="Warning banner" status="warning">
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
<InfoCircleIcon />
<ExclamationTriangleIcon />
</FlexItem>
<FlexItem>Info banner</FlexItem>
<FlexItem>Warning banner</FlexItem>
</Flex>
</Banner>
<br />
<Banner screenReaderText="Danger banner" variant="red">
<Banner screenReaderText="Danger banner" status="danger">
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
<ExclamationCircleIcon />
Expand All @@ -35,21 +35,21 @@ export const BannerStatus: React.FunctionComponent = () => (
</Flex>
</Banner>
<br />
<Banner screenReaderText="Success banner" variant="green">
<Banner screenReaderText="Info banner" status="info">
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
<CheckCircleIcon />
<InfoCircleIcon />
</FlexItem>
<FlexItem>Success banner</FlexItem>
<FlexItem>Info banner</FlexItem>
</Flex>
</Banner>
<br />
<Banner screenReaderText="Warning banner" variant="gold">
<Banner screenReaderText="Custom banner" status="custom">
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
<ExclamationTriangleIcon />
<BellIcon />
</FlexItem>
<FlexItem>Warning banner</FlexItem>
<FlexItem>Custom banner</FlexItem>
</Flex>
</Banner>
</>
Expand Down